Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals

[Markus Armbruster]

Eduardo Habkost  writes:

> On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote:
[...]
>> When the structure of a data type is to be kept away from its users, I
>> prefer to keep it out of the public header, so the compiler enforces the
>> encapsulation.
>
> I prefer that too, except that it is impossible when users of the
> API need the compiler to know the struct size.

There are cases where the structure of a data type should be
encapsulated, yet its size must be made known for performance (avoid
dynamic memory allocation and pointer chasing).

Need for encapsulation correlates with complex algorithms and data
structures.  The cost of dynamic allocation is often in the noise then.

Re: [PATCH] pci/shpc: don't push attention button when ejecting powered-off device

[Roman Kagan]

On Mon, Nov 02, 2020 at 08:37:50AM +0300, Roman Kagan wrote:
> When the slot is in steady powered-off state and the device is being
> removed, there's no need to press the attention button.  Nor is it
> mandated by the Standard Hot-Plug Controller Specification, Rev. 1.0.
> 
> Moreover it confuses the guest, Linux in particular, as it assumes that
> the attention button pressed in this state indicates that the device has
> been inserted and will need to be powered on.  Therefore it transitions
> the slot into BLINKING_ON state for 5 seconds, and discovers at the end
> that no device is actually inserted:
> 
> ... unplug request
> [12685.451329] shpchp :01:00.0: Button pressed on Slot(2)
> [12685.455478] shpchp :01:00.0: PCI slot #2 - powering off due to button 
> press
> ... in 5 seconds OS powers off the slot, QEMU ejects the device
> [12690.632282] shpchp :01:00.0: Latch open on Slot(2)
> ... excessive button press in steady powered-off state
> [12690.634267] shpchp :01:00.0: Button pressed on Slot(2)
> [12690.636256] shpchp :01:00.0: Card not present on Slot(2)
> ... the last button press spawns powering on the slot
> [12690.638909] shpchp :01:00.0: PCI slot #2 - powering on due to button 
> press
> ... in 5 more seconds attempt to power on discovers empty slot
> [12695.735986] shpchp :01:00.0: No adapter on slot(2)
> 
> Worse, if the real device insertion happens within 5 seconds from the
> apparent completion of the previous device removal (signaled via
> DEVICE_DELETED event), the new button press will be interpreted as the
> cancellation of that misguided powering on:
> 
> [13448.965295] shpchp :01:00.0: Button pressed on Slot(2)
> [13448.969430] shpchp :01:00.0: PCI slot #2 - powering off due to button 
> press
> [13454.025107] shpchp :01:00.0: Latch open on Slot(2)
> [13454.027101] shpchp :01:00.0: Button pressed on Slot(2)
> [13454.029165] shpchp :01:00.0: Card not present on Slot(2)
> ... the excessive button press spawns powering on the slot
> ... device has already been ejected by QEMU
> [13454.031949] shpchp :01:00.0: PCI slot #2 - powering on due to button 
> press
> ... new device is inserted in the slot
> [13456.861545] shpchp :01:00.0: Latch close on Slot(2)
> ... valid button press arrives before 5 s since the wrong one
> [13456.864894] shpchp :01:00.0: Button pressed on Slot(2)
> [13456.869211] shpchp :01:00.0: Card present on Slot(2)
> ... the valid button press is counted as cancellation of the wrong one
> [13456.873173] shpchp :01:00.0: Button cancel on Slot(2)
> [13456.877101] shpchp :01:00.0: PCI slot #2 - action canceled due to 
> button press
> 
> As a result, the newly inserted device isn't brought up by the guest.
> 
> Avoid this situation by not pushing the attention button when the device
> in the slot is in powered-off state and is being ejected.
> 
> FWIW pcie implementation doesn't suffer from this problem.
> 
> Signed-off-by: Roman Kagan 
> ---
>  hw/pci/shpc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
> index b00dce629c..837159c5bd 100644
> --- a/hw/pci/shpc.c
> +++ b/hw/pci/shpc.c
> @@ -300,7 +300,6 @@ static void shpc_slot_command(SHPCDevice *shpc, uint8_t 
> target,
>  shpc_set_status(shpc, slot, SHPC_SLOT_STATUS_PRSNT_EMPTY,
>  SHPC_SLOT_STATUS_PRSNT_MASK);
>  shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |=
> -SHPC_SLOT_EVENT_BUTTON |
>  SHPC_SLOT_EVENT_MRL |
>  SHPC_SLOT_EVENT_PRESENCE;
>  }
> @@ -566,7 +565,6 @@ void shpc_device_unplug_request_cb(HotplugHandler 
> *hotplug_dev,
>  return;
>  }
>  
> -shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |= SHPC_SLOT_EVENT_BUTTON;
>  state = shpc_get_status(shpc, slot, SHPC_SLOT_STATE_MASK);
>  led = shpc_get_status(shpc, slot, SHPC_SLOT_PWR_LED_MASK);
>  if (state == SHPC_STATE_DISABLED && led == SHPC_LED_OFF) {
> @@ -577,6 +575,8 @@ void shpc_device_unplug_request_cb(HotplugHandler 
> *hotplug_dev,
>  shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |=
>  SHPC_SLOT_EVENT_MRL |
>  SHPC_SLOT_EVENT_PRESENCE;
> +} else {
> +shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |= SHPC_SLOT_EVENT_BUTTON;
>  }
>  shpc_set_status(shpc, slot, 0, SHPC_SLOT_STATUS_66);
>  shpc_interrupt_update(pci_hotplug_dev);
> -- 
> 2.28.0
> 

Ping?

Re: [PATCH v2 2/2] pc-bios: s390x: Clear out leftover S390EP string

[Christian Borntraeger]

On 20.11.20 17:01, Eric Farman wrote:
> A Linux binary will have the string "S390EP" at address 0x10008,
> which is important in getting the guest up off the ground. In the
> case of a reboot (specifically chreipl going to a new device),
> we should defer to the PSW at address zero for the new config,
> which will re-write "S390EP" from the new image.
> 
> Let's clear it out at this point so that a reipl to, say, a DASD
> passthrough device drives the IPL path from scratch without disrupting
> disrupting the order of operations for other boots.
> 
> Rather than hardcoding the address of this magic (again), let's
> define it somewhere so that the two users are visibly related.


Hmmm, this might have side effects, e.g. if you do something like a kdump
or kexec to a non-Linux binary that happens to have code at 0x10008, no?

As far as I can tell, the problem should only happen for a ccw type IPL
so why not

[...]
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -178,6 +178,12 @@ static void boot_setup(void)
>  memcpy(lpmsg + 10, loadparm_str, 8);
>  sclp_print(lpmsg);
>  
> +/*
> + * Clear out any potential S390EP magic (see jump_to_low_kernel()),
> + * so we don't taint our decision-making process during a reboot.
> + */
> +memset((char *)S390EP, 0, 6);


move this into find_subch
in here:
- snip ---
case CU_TYPE_DASD_3990:
case CU_TYPE_DASD_2107:
return true;
- snip ---

[PATCH 3/3] hw/block/nvme: allow cmb and pmr to coexist

[Klaus Jensen]

From: Klaus Jensen 

With BAR 4 now free to use, allow PMR and CMB to be enabled
simultaneously.

Signed-off-by: Klaus Jensen 
---
 hw/block/nvme.c | 17 -
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index db8c5ae2f527..72d5449121c7 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -28,14 +28,13 @@
  * Note cmb_size_mb denotes size of CMB in MB. CMB is assumed to be at
  * offset 0 in BAR2 and supports only WDS, RDS and SQS for now.
  *
- * cmb_size_mb= and pmrdev= options are mutually exclusive due to limitation
- * in available BAR's. cmb_size_mb= will take precedence over pmrdev= when
- * both provided.
  * Enabling pmr emulation can be achieved by pointing to memory-backend-file.
  * For example:
  * -object memory-backend-file,id=,share=on,mem-path=, \
  *  size=  -device nvme,...,pmrdev=
  *
+ * The PMR will use BAR 4/5 exclusively.
+ *
  *
  * nvme device parameters
  * ~~
@@ -76,7 +75,7 @@
 #define NVME_DB_SIZE  4
 #define NVME_SPEC_VER 0x00010300
 #define NVME_CMB_BIR 2
-#define NVME_PMR_BIR 2
+#define NVME_PMR_BIR 4
 #define NVME_TEMPERATURE 0x143
 #define NVME_TEMPERATURE_WARNING 0x157
 #define NVME_TEMPERATURE_CRITICAL 0x175
@@ -2520,7 +2519,7 @@ static void nvme_check_constraints(NvmeCtrl *n, Error 
**errp)
 return;
 }
 
-if (!n->params.cmb_size_mb && n->pmrdev) {
+if (n->pmrdev) {
 if (host_memory_backend_is_mapped(n->pmrdev)) {
 error_setg(errp, "can't use already busy memdev: %s",
object_get_canonical_path_component(OBJECT(n->pmrdev)));
@@ -2610,9 +2609,6 @@ static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev)
 
 static void nvme_init_pmr(NvmeCtrl *n, PCIDevice *pci_dev)
 {
-/* Controller Capabilities register */
-NVME_CAP_SET_PMRS(n->bar.cap, 1);
-
 /* PMR Capabities register */
 n->bar.pmrcap = 0;
 NVME_PMRCAP_SET_RDS(n->bar.pmrcap, 0);
@@ -2705,7 +2701,9 @@ static void nvme_init_pci(NvmeCtrl *n, PCIDevice 
*pci_dev, Error **errp)
 
 if (n->params.cmb_size_mb) {
 nvme_init_cmb(n, pci_dev);
-} else if (n->pmrdev) {
+}
+
+if (n->pmrdev) {
 nvme_init_pmr(n, pci_dev);
 }
 }
@@ -2775,6 +2773,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice 
*pci_dev)
 NVME_CAP_SET_CSS(n->bar.cap, NVME_CAP_CSS_ADMIN_ONLY);
 NVME_CAP_SET_MPSMAX(n->bar.cap, 4);
 NVME_CAP_SET_CMBS(n->bar.cap, n->params.cmb_size_mb ? 1 : 0);
+NVME_CAP_SET_PMRS(n->bar.cap, n->pmrdev ? 1 : 0);
 
 n->bar.vs = NVME_SPEC_VER;
 n->bar.intmc = n->bar.intms = 0;
-- 
2.29.2

[PATCH 2/3] hw/block/nvme: move msix table and pba to BAR 0

[Klaus Jensen]

From: Klaus Jensen 

In the interest of supporting both CMB and PMR to be enabled on the same
device, move the MSI-X table and pending bit out of BAR 4 and into BAR
0.

This is a simplified version of the patch contributed by Andrzej
Jakowski (see [1]). Leaving the CMB at offset 0 removes the need for
changes to CMB address mapping code.

  [1]: 
https://lore.kernel.org/qemu-devel/20200729220107.37758-3-andrzej.jakow...@linux.intel.com/

Signed-off-by: Klaus Jensen 
---
 hw/block/nvme.h |  1 +
 hw/block/nvme.c | 24 ++--
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index e080a2318a50..b7b42e70f97f 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -116,6 +116,7 @@ typedef struct NvmeFeatureVal {
 
 typedef struct NvmeCtrl {
 PCIDeviceparent_obj;
+MemoryRegion bar0;
 MemoryRegion iomem;
 MemoryRegion ctrl_mem;
 NvmeBar  bar;
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 4c599159f533..db8c5ae2f527 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -2660,6 +2660,8 @@ static void nvme_init_pmr(NvmeCtrl *n, PCIDevice *pci_dev)
 static void nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
 {
 uint8_t *pci_conf = pci_dev->config;
+uint64_t bar_size, msix_table_size, msix_pba_size;
+unsigned msix_table_offset, msix_pba_offset;
 
 pci_conf[PCI_INTERRUPT_PIN] = 1;
 pci_config_set_prog_interface(pci_conf, 0x2);
@@ -2675,11 +2677,29 @@ static void nvme_init_pci(NvmeCtrl *n, PCIDevice 
*pci_dev, Error **errp)
 pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_EXPRESS);
 pcie_endpoint_cap_init(pci_dev, 0x80);
 
+bar_size = QEMU_ALIGN_UP(n->reg_size, 4 * KiB);
+msix_table_offset = bar_size;
+msix_table_size = PCI_MSIX_ENTRY_SIZE * n->params.msix_qsize;
+
+bar_size += msix_table_size;
+bar_size = QEMU_ALIGN_UP(bar_size, 4 * KiB);
+msix_pba_offset = bar_size;
+msix_pba_size = QEMU_ALIGN_UP(n->params.msix_qsize, 64) / 8;
+
+bar_size += msix_pba_size;
+bar_size = pow2ceil(bar_size);
+
+memory_region_init(>bar0, OBJECT(n), "nvme-bar0", bar_size);
 memory_region_init_io(>iomem, OBJECT(n), _mmio_ops, n, "nvme",
   n->reg_size);
+memory_region_add_subregion(>bar0, 0, >iomem);
+
 pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |
- PCI_BASE_ADDRESS_MEM_TYPE_64, >iomem);
-if (msix_init_exclusive_bar(pci_dev, n->params.msix_qsize, 4, errp)) {
+ PCI_BASE_ADDRESS_MEM_TYPE_64, >bar0);
+
+if (msix_init(pci_dev, n->params.msix_qsize,
+  >bar0, 0, msix_table_offset,
+  >bar0, 0, msix_pba_offset, 0, errp)) {
 return;
 }
 
-- 
2.29.2

[PATCH 0/3] hw/block/nvme: allow cmb and pmr to coexist

[Klaus Jensen]

From: Klaus Jensen 

This is a resurrection of Andrzej's series[1] from back July.

Andrzej's main patch basically moved the the CMB from BAR 2 into an
offset in BAR 4 (located after the MSI-X table and PBA). Having an
offset on the CMB causes a bunch of calculations related to address
mapping to change.

So, since I couldn't get the patch to apply cleanly I took a stab at
implementing the suggestion I originally came up with: simply move the
MSI-X table and PBA from BAR 4 into BAR 0 (up-aligned to a 4 KiB
boundary, after the main NVMe controller registers). This way we can
keep the CMB at offset zero in its own BAR and free up BAR 4 for use by
PMR. This makes the patch simpler and does not impact any of the
existing address mapping code.

Andrzej, I would prefer an Ack from you, since I pretty much voided your
main patch.

  [1]: 
https://lore.kernel.org/qemu-devel/20200729220107.37758-1-andrzej.jakow...@linux.intel.com/

CC: Andrzej Jakowski 

Andrzej Jakowski (1):
  hw/block/nvme: indicate CMB support through controller capabilities
register

Klaus Jensen (2):
  hw/block/nvme: move msix table and pba to BAR 0
  hw/block/nvme: allow cmb and pmr to coexist

 hw/block/nvme.h  |  1 +
 include/block/nvme.h | 10 +++---
 hw/block/nvme.c  | 42 +++---
 3 files changed, 39 insertions(+), 14 deletions(-)

-- 
2.29.2

[PATCH 1/3] hw/block/nvme: indicate CMB support through controller capabilities register

[Klaus Jensen]

From: Andrzej Jakowski 

This patch sets CMBS bit in controller capabilities register when user
configures NVMe driver with CMB support, so capabilites are correctly
reported to guest OS.

Signed-off-by: Andrzej Jakowski 
Reviewed-by: Maxim Levitsky 
Signed-off-by: Klaus Jensen 
---
 include/block/nvme.h | 10 +++---
 hw/block/nvme.c  |  1 +
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/include/block/nvme.h b/include/block/nvme.h
index 8a46d9cf015f..c7cba786b491 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -36,6 +36,7 @@ enum NvmeCapShift {
 CAP_MPSMIN_SHIFT   = 48,
 CAP_MPSMAX_SHIFT   = 52,
 CAP_PMR_SHIFT  = 56,
+CAP_CMB_SHIFT  = 57,
 };
 
 enum NvmeCapMask {
@@ -49,6 +50,7 @@ enum NvmeCapMask {
 CAP_MPSMIN_MASK= 0xf,
 CAP_MPSMAX_MASK= 0xf,
 CAP_PMR_MASK   = 0x1,
+CAP_CMB_MASK   = 0x1,
 };
 
 #define NVME_CAP_MQES(cap)  (((cap) >> CAP_MQES_SHIFT)   & CAP_MQES_MASK)
@@ -78,9 +80,11 @@ enum NvmeCapMask {
 #define NVME_CAP_SET_MPSMIN(cap, val) (cap |= (uint64_t)(val & 
CAP_MPSMIN_MASK)\
<< CAP_MPSMIN_SHIFT)
 #define NVME_CAP_SET_MPSMAX(cap, val) (cap |= (uint64_t)(val & 
CAP_MPSMAX_MASK)\
-<< 
CAP_MPSMAX_SHIFT)
-#define NVME_CAP_SET_PMRS(cap, val) (cap |= (uint64_t)(val & CAP_PMR_MASK)\
-<< CAP_PMR_SHIFT)
+   << CAP_MPSMAX_SHIFT)
+#define NVME_CAP_SET_PMRS(cap, val)   (cap |= (uint64_t)(val & CAP_PMR_MASK)   
\
+   << CAP_PMR_SHIFT)
+#define NVME_CAP_SET_CMBS(cap, val)   (cap |= (uint64_t)(val & CAP_CMB_MASK)   
\
+   << CAP_CMB_SHIFT)
 
 enum NvmeCapCss {
 NVME_CAP_CSS_NVM= 1 << 0,
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 01b657b1c5e2..4c599159f533 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -2754,6 +2754,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice 
*pci_dev)
 NVME_CAP_SET_CSS(n->bar.cap, NVME_CAP_CSS_NVM);
 NVME_CAP_SET_CSS(n->bar.cap, NVME_CAP_CSS_ADMIN_ONLY);
 NVME_CAP_SET_MPSMAX(n->bar.cap, 4);
+NVME_CAP_SET_CMBS(n->bar.cap, n->params.cmb_size_mb ? 1 : 0);
 
 n->bar.vs = NVME_SPEC_VER;
 n->bar.intmc = n->bar.intms = 0;
-- 
2.29.2

Re: QMP and the 'id' parameter

[Markus Armbruster]

John Snow  writes:

> On 11/20/20 5:25 AM, Markus Armbruster wrote:
>> John Snow  writes:
>>> On 11/11/20 3:27 AM, Markus Armbruster wrote:
 John Snow  writes:
> On 11/10/20 1:22 AM, Markus Armbruster wrote:
>> John Snow  writes:
>
>> 
>> If you find yourself in a situation where an error class would
>> definitely be useful, we should talk about providing you one.
>> 
>
> OK, thanks. I just wanted to check there wasn't a stronger opposition 
> than I was aware of. I only vaguely knew they were "on the way out."
>
 QGA has a special case, but you probably don't want to know.
>>>
>>> I do want to know, unfortunately. A good QMP client should likely
>>> support both targets.
>> 
>> QGA has a few commands that send *nothing* on success.
>> 
>> qapi-code-gen.txt explains:
>> 
>>  Normally, the QAPI schema is used to describe synchronous exchanges,
>>  where a response is expected.  But in some cases, the action of a
>>  command is expected to change state in a way that a successful
>>  response is not possible (although the command will still return an
>>  error object on failure).  When a successful reply is not possible,
>>  the command definition includes the optional member 'success-response'
>>  with boolean value false.  So far, only QGA makes use of this member.
>> 
>> qmp-spec.txt neglects to cover this special case.
>> 
>
> Oh, I see. That's fun!
>
> (Was it not possible to have the client send an ACK response that 
> doesn't indicate success, but just signals receipt? Semantically, it 
> would be the difference between do-x and start-x as a command.)

Feels quite possible to me.

If I read git-log correctly, the commands' design ignored the race with
shutdown / suspend, and 'success-response': false was a quick fix.

I think changing the commands from 'do-x' to 'start-x' would have been
better.  A bit more work, though, and back then there weren't any
'start-x' examples to follow, I guess.

I wish success-response didn't exist, but is getting rid of it worth
changing the interface?  I honestly don't know.

 Even though counting responses is a workable way to map responses back
 to requests, I recommend to either have at most one request in flight
 (so no counting is necessary), or to add IDs to all requests (so
 counting isn't necessary as long as you get the syntax right enough to
 avoid the ID-less errors discussed above).
>>>
>>> I am taking this approach. I still need some error handling for the
>>> case that an incoming response cannot be routed to the correct pending
>>> queue.
>>>
>>> The QMP spec states that if I get an ID and I don't recognize it, I am
>>> free to drop the response on the floor (So I have done so),
>> 
>> Should not happen.  When it does, something is wrong, possibly seriously
>> wrong.
>
> Yep, I agree.
>
> (In my client library: I have decided to enforce string IDs and not 
> allow the caller to specify their own. Though we allow arbitrary objects 
> to be used as an ID, being able to compare more complex objects seems 
> more prone to failure than a simple string.)

Makes sense.

>> One possible scenario: ID got corrupted on the way from client via
>> server back to the client.  The response with the uncorrupted ID will
>> never come.  Other responses and events may still arrive fine.
>> 
>> Two recovery strategies:
>> 
>> 1. Drop the unexpected response, carry on.  If commands are in flight,
>> be prepared for missing responses.
>> 
>> 2. Give up, close connection.
>> 
>> Pick the strategy that best fits your client's needs.
>
> Dropping it on the floor seems fine for now, the spec says I can! :)
>
> The most realistic scenario for this outside of catastrophic error is 
> that a client sends a command, but then times out waiting for a 
> response. Later, the response arrives -- but that context has already 
> come and gone, so there's nowhere to route the message to.
>
> Effectively, an orphaned reply. I suppose I could always stash them in 
> an orphaned queue and a very, very rigorous client could check the 
> orphaned queue if something winds up in there, but I suspect most 
> clients would actually be just as happy to let it fall on the floor.

Sounds okay to me.

>>>  but I need
>>> to handle the case that I see a response with no ID at all.
>> 
>> Should not happen as long as the client sends syntactically sane input.
>> When it does, something is wrong, possibly seriously wrong.
>> 
>>> Perhaps the easiest, and most sensible thing to do, is to declare this
>>> a catastrophic failure and move the QMP connection into an error
>>> state, invalidate the whole queue, and disconnect.
>>>
>>> (And then maybe some of the commands you issued got processed, maybe
>>> they didn't. I guess it's up to the application driving the library to
>>> query around to find out what happened.)
>> 
>> Again, two recovery strategies:
>> 
>> 1. Drop the unexpected 

Re: [PATCH v2] usb: fix kconfig for usb-xhci-sysbus

[Gerd Hoffmann]

On Fri, Nov 20, 2020 at 10:45:06AM -0500, Paolo Bonzini wrote:
> Remove the "default y" for USB_XHCI_SYSBUS because
> sysbus devices are not user creatable; boards that use them will
> specify them manually with "imply" or "select" clauses.
> 
> It would be nice to keep the ability to remove PCIe and USB from microvm,
> since thos can be disabled on the command line and therefore should not
> be included if QEMU is configured --without-default-devices.  However
> it's too late for 5.2 to figure out a place for the DSDT creation code.

Reviewed-by: Gerd Hoffmann 

Re: [PATCH v2 0/6] spapr/xive: Activate StoreEOI in P10 compat guests

[David Gibson]

On Mon, Nov 02, 2020 at 02:22:35PM +0100, Cédric Le Goater wrote:
> Sorry for the late answer I was out for a couple of weeks.
> 
> On 10/9/20 2:23 AM, David Gibson wrote:
> > On Mon, Oct 05, 2020 at 06:51:41PM +0200, Cédric Le Goater wrote:
> >> Hello,
> >>
> >> When an interrupt has been handled, the OS notifies the interrupt
> >> controller with an EOI sequence. On the XIVE interrupt controller
> >> (POWER9 and POWER10), this can be done with a load or a store
> >> operation on the ESB interrupt management page of the interrupt. The
> >> StoreEOI operation has less latency and improves interrupt handling
> >> performance but it was deactivated during the POWER9 DD2.0 time-frame
> >> because of ordering issues. POWER9 systems use the LoadEOI instead.
> >> POWER10 has fixed the issue with a special load command which enforces
> >> Load-after-Store ordering and StoreEOI can be safely used.
> > 
> > Do you mean that ordering is *always* enforced on P10?  Or it's a
> > special form of load that has the ordering?
> 
> It's a special load offset that has the ordering. Oring 0x40 to the load
> address : 
> 
>   #define XIVE_ESB_LOAD_EOI   0x000 /* Load */
>   #define XIVE_ESB_GET0x800 /* Load */
>   #define XIVE_ESB_SET_PQ_00  0xc00 /* Load */
>   #define XIVE_ESB_SET_PQ_01  0xd00 /* Load */
>   #define XIVE_ESB_SET_PQ_10  0xe00 /* Load */
>   #define XIVE_ESB_SET_PQ_11  0xf00 /* Load */
> 
> will enforce load-after-store ordering.

Oh... I had assumed the problem was to do with the load/store ordering
within the CPU core itself (or maybe the L1, I guess).  But if the
address used can change it, the problem must be within the XIVE, yes?
Or at least somwhere on the Powerbus.  So, wasn't this just a plain
XIVE hardware bug?  In which case why is there software involvement as
well?

> We only need it for XIVE_ESB_SET_PQ_10. See commit b1f9be9392f0 
> ("powerpc/xive: Enforce load-after-store ordering when StoreEOI is active") 
> in Linux.
> 
> C. 
> 
> 
> > 
> > Also, weirdly, despite the series being addressed to me, only some of
> > the patches ended up in my inbox, rather than the list folder :/.
> > 
> >> These changes add a new StoreEOI capability which activate StoreEOI
> >> support in the flags returned by the hcall H_INT_GET_SOURCE_INFO. When
> >> the machine is using an emulated interrupt controller, TCG or without
> >> kernel IRQ chip, there are no limitations and activating StoreEOI is
> >> not an issue. However, when running with a kernel IRQ chip, some
> >> verification needs to be done on the host. This is done through the
> >> DT, which tells us that firmware has configured the HW for StoreEOI,
> >> but a new KVM capability would be cleaner.
> >>
> >> The last patch introduces a new 'cas' value to the capability which
> >> lets the hypervisor decide at CAS time if StoreEOI should be
> >> advertised to the guest OS. P10 compat kernel are considered safe
> >> because the OS enforces load-after-store ordering but not with P9.
> >>
> >> The StoreEOI capability is a global setting and does not take into
> >> account the characteristics of a single source. It could be an issue
> >> if StoreEOI is not supported on a specific source, of a passthrough
> >> device for instance. In that case, we could either introduce a new KVM
> >> ioctl to query the characteristics of the source at the HW level (like
> >> in v1) or deactivate StoreEOI on the machine.
> >>
> >> We are using these patches today on P10 and P9 (with a custom FW
> >> activating StoreEOI) systems to benchmark interrupt performance on
> >> large guests but there's no hurry to take them. Let's discuss this new
> >> approach.
> >>
> >> Thanks,
> >>
> >> C.
> >>
> >> Changes in v2:
> >>
> >>  - completely approach using a capability
> >>
> >> Cédric Le Goater (6):
> >>   spapr/xive: Introduce a StoreEOI capability
> >>   spapr/xive: Add a warning when StoreEOI is activated on POWER8 CPUs
> >>   spapr/xive: Add a warning when StoreEOI is activated on POWER9 CPUs
> >>   spapr/xive: Enforce load-after-store ordering
> >>   spapr/xive: Activate StoreEOI at the source level
> >>   spapr/xive: Introduce a new CAS value for the StoreEOI capability
> >>
> >>  include/hw/ppc/spapr.h  |  5 +++-
> >>  include/hw/ppc/spapr_xive.h |  1 +
> >>  include/hw/ppc/xive.h   |  8 +
> >>  target/ppc/kvm_ppc.h|  6 
> >>  hw/intc/spapr_xive.c| 10 +++
> >>  hw/intc/spapr_xive_kvm.c| 12 
> >>  hw/intc/xive.c  |  6 
> >>  hw/ppc/spapr.c  |  1 +
> >>  hw/ppc/spapr_caps.c | 60 +
> >>  hw/ppc/spapr_hcall.c|  7 +
> >>  hw/ppc/spapr_irq.c  |  6 
> >>  target/ppc/kvm.c| 18 +++
> >>  12 files changed, 139 insertions(+), 1 deletion(-)
> >>
> > 
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
  

[PATCH] net: Use correct default-path macro for downscript

[Keqian Zhu]

Fixes: 63c4db4c2e6d (net: relocate paths to helpers and scripts)
Signed-off-by: Keqian Zhu 
---
 net/tap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/tap.c b/net/tap.c
index c46ff66184..b8e5cca51c 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -951,7 +951,8 @@ free_fail:
 script = default_script = 
get_relocated_path(DEFAULT_NETWORK_SCRIPT);
 }
 if (!downscript) {
-downscript = default_downscript = 
get_relocated_path(DEFAULT_NETWORK_SCRIPT);
+downscript = default_downscript =
+ 
get_relocated_path(DEFAULT_NETWORK_DOWN_SCRIPT);
 }
 
 if (tap->has_ifname) {
-- 
2.23.0

Re: [PATCH 1/2] ppc/translate: Implement lxvwsx opcode

[David Gibson]

On Sat, Nov 14, 2020 at 09:13:33AM -0800, Richard Henderson wrote:
> On 11/10/20 10:22 PM, David Gibson wrote:
> > On Tue, Nov 10, 2020 at 10:14:23AM +0100, LemonBoy wrote:
> >> Is there any chance for this patch series to be merged for 5.2?
> > 
> > No.  We are now in hard freeze, and this is not a bugfix.
> 
> Actually, patch 1/2 is a bugfix -- a missing instruction from an ISA that we
> claim to implement.

Fair point.  I'm applying 1/2 to ppc-for-5.2 and 2/2 to ppc-for-6.0.

> 
> 
> r~
> 
> > 
> >>
> >> On 09/11/20 18:39, Richard Henderson wrote:
> >>> On 11/9/20 1:17 AM, LemonBoy wrote:
>  Implement the "Load VSX Vector Word & Splat Indexed" opcode, introduced
>  in Power ISA v3.0.
> 
>  Buglink: https://bugs.launchpad.net/qemu/+bug/1793608
>  Signed-off-by: Giuseppe Musacchio 
>  ---
>   target/ppc/translate/vsx-impl.c.inc | 30 +
>   target/ppc/translate/vsx-ops.c.inc  |  1 +
>   2 files changed, 31 insertions(+)
> >>>
> >>> Reviewed-by: Richard Henderson 
> >>>
> >>> r~
> >>>
> >>
> > 
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH v3 6/7] ppc: Add a missing break for PPC6xx_INPUT_TBEN

[David Gibson]

On Mon, Nov 16, 2020 at 10:48:09AM +0800, Chen Qun wrote:
> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
> hw/ppc/ppc.c: In function ‘ppc6xx_set_irq’:
> hw/ppc/ppc.c:118:16: warning: this statement may fall through 
> [-Wimplicit-fallthrough=]
>   118 | if (level) {
>   |^
> hw/ppc/ppc.c:123:9: note: here
>   123 | case PPC6xx_INPUT_INT:
>   | ^~~~
> 
> According to the discussion, a break statement needs to be added here.
> 
> Reported-by: Euler Robot 
> Signed-off-by: Chen Qun 
> Reviewed-by: Thomas Huth 
> Acked-by: David Gibson 

Applied to ppc-for-6.0, thanks.

> ---
> v1->v2: Add a "break" statement here instead of /* fall through */ comments
> (Base on Thomas's and David review).
> 
> Cc: Thomas Huth 
> Cc: David Gibson 
> ---
>  hw/ppc/ppc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index 4a11fb1640..1b98272076 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -120,6 +120,7 @@ static void ppc6xx_set_irq(void *opaque, int pin, int 
> level)
>  } else {
>  cpu_ppc_tb_stop(env);
>  }
> +break;
>  case PPC6xx_INPUT_INT:
>  /* Level sensitive - active high */
>  LOG_IRQ("%s: set the external IRQ state to %d\n",

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH v3 7/7] target/ppc: replaced the TODO with LOG_UNIMP and add break for silence warnings

[David Gibson]

On Mon, Nov 16, 2020 at 10:48:10AM +0800, Chen Qun wrote:
> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
> target/ppc/mmu_helper.c: In function ‘dump_mmu’:
> target/ppc/mmu_helper.c:1351:12: warning: this statement may fall through 
> [-Wimplicit-fallthrough=]
>  1351 | if (ppc64_v3_radix(env_archcpu(env))) {
>   |^
> target/ppc/mmu_helper.c:1358:5: note: here
>  1358 | default:
>   | ^~~
> 
> Use "qemu_log_mask(LOG_UNIMP**)" instead of the TODO comment.
> And add the break statement to fix it.
> 
> Reported-by: Euler Robot 
> Signed-off-by: Chen Qun 
> Reviewed-by: Philippe Mathieu-Daudé 
> Reviewed-by: Thomas Huth 
> Acked-by: David Gibson 

Applied to ppc-for-6.0, thanks.

> ---
> v1->v2: replace the TODO by a LOG_UNIMP call and add break statement(Base on 
> Philippe's comments)
> 
> Cc: Thomas Huth 
> Cc: David Gibson 
> Cc: Philippe Mathieu-Daudé 
> ---
>  target/ppc/mmu_helper.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> index 8972714775..3bb50546f8 100644
> --- a/target/ppc/mmu_helper.c
> +++ b/target/ppc/mmu_helper.c
> @@ -1349,11 +1349,12 @@ void dump_mmu(CPUPPCState *env)
>  break;
>  case POWERPC_MMU_3_00:
>  if (ppc64_v3_radix(env_archcpu(env))) {
> -/* TODO - Unsupported */
> +qemu_log_mask(LOG_UNIMP, "%s: the PPC64 MMU is unsupported\n",
> +  __func__);
>  } else {
>  dump_slb(env_archcpu(env));
> -break;
>  }
> +break;
>  #endif
>  default:
>  qemu_log_mask(LOG_UNIMP, "%s: unimplemented\n", __func__);

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH v3 0/4] ppc/translate: Fix unordered f64/f128 comparisons

[David Gibson]

On Fri, Nov 13, 2020 at 12:01:26AM +0100, LemonBoy wrote:
> Fix a couple of problems found in the emulation of f64/f128 comparisons plus
> some minimal self-contained commits to clean-up some code.

Applied to ppc-for-6.0.

As bug fixes, these could theoretically go into qemu-5.2.  However,
since they're not regressions, I'm more comfortable delaying these to
6.0 given how late we are in the qemu-5.2 cycle.

> 
> Giuseppe Musacchio (4):
>   ppc/translate: Fix unordered f64/f128 comparisons
>   ppc/translate: Turn the helper macros into functions
>   ppc/translate: Delay NaN checking after comparison
>   ppc/translate: Raise exceptions after setting the cc
> 
>  target/ppc/fpu_helper.c | 212 +++-
>  1 file changed, 121 insertions(+), 91 deletions(-)
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH-for-5.2 v3 6/7] ppc: Add a missing break for PPC6xx_INPUT_TBEN

[David Gibson]

On Mon, Nov 16, 2020 at 12:46:32PM +0100, Philippe Mathieu-Daudé wrote:
> David, can you queue this patch for 5.2 (bugfix)?

Sorry about this, I've been on vacation.

Although it is a bugfix, it's been there for a very long time and
no-one's hit it in practice.

So, I'm disinclined to push it in this late in the 5.2 cycle.

> 
> On 11/16/20 3:48 AM, Chen Qun wrote:
> > When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed 
> > warning:
> > hw/ppc/ppc.c: In function ‘ppc6xx_set_irq’:
> > hw/ppc/ppc.c:118:16: warning: this statement may fall through 
> > [-Wimplicit-fallthrough=]
> >   118 | if (level) {
> >   |^
> > hw/ppc/ppc.c:123:9: note: here
> >   123 | case PPC6xx_INPUT_INT:
> >   | ^~~~
> > 
> > According to the discussion, a break statement needs to be added here.
> > 
> > Reported-by: Euler Robot 
> > Signed-off-by: Chen Qun 
> > Reviewed-by: Thomas Huth 
> > Acked-by: David Gibson 
> > ---
> > v1->v2: Add a "break" statement here instead of /* fall through */ comments
> > (Base on Thomas's and David review).
> > 
> > Cc: Thomas Huth 
> > Cc: David Gibson 
> > ---
> >  hw/ppc/ppc.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> > index 4a11fb1640..1b98272076 100644
> > --- a/hw/ppc/ppc.c
> > +++ b/hw/ppc/ppc.c
> > @@ -120,6 +120,7 @@ static void ppc6xx_set_irq(void *opaque, int pin, int 
> > level)
> >  } else {
> >  cpu_ppc_tb_stop(env);
> >  }
> > +break;
> >  case PPC6xx_INPUT_INT:
> >  /* Level sensitive - active high */
> >  LOG_IRQ("%s: set the external IRQ state to %d\n",
> > 
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH for-6.0 5/9] spapr: Simplify error path of spapr_core_plug()

[David Gibson]

On Sat, Nov 21, 2020 at 12:42:04AM +0100, Greg Kurz wrote:
> spapr_core_pre_plug() already guarantees that the slot for the given core
> ID is available. It is thus safe to assume that spapr_find_cpu_slot()
> returns a slot during plug. Turn the error path into an assertion.
> It is also safe to assume that no device is attached to the corresponding
> DRC and that spapr_drc_attach() shouldn't fail.
> 
> Pass _abort to spapr_drc_attach() and simplify error handling.
> 
> Signed-off-by: Greg Kurz 

Applied to ppc-for-6.0, thanks.

> ---
>  hw/ppc/spapr.c | 21 ++---
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index da7586f548df..cfca033c7b14 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3739,8 +3739,7 @@ int spapr_core_dt_populate(SpaprDrc *drc, 
> SpaprMachineState *spapr,
>  return 0;
>  }
>  
> -static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> -Error **errp)
> +static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
>  {
>  SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
>  MachineClass *mc = MACHINE_GET_CLASS(spapr);
> @@ -3755,20 +3754,20 @@ static void spapr_core_plug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  int i;
>  
>  core_slot = spapr_find_cpu_slot(MACHINE(hotplug_dev), cc->core_id, 
> );
> -if (!core_slot) {
> -error_setg(errp, "Unable to find CPU core with core-id: %d",
> -   cc->core_id);
> -return;
> -}
> +g_assert(core_slot); /* Already checked in spapr_core_pre_plug() */
> +
>  drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU,
>spapr_vcpu_id(spapr, cc->core_id));
>  
>  g_assert(drc || !mc->has_hotpluggable_cpus);
>  
>  if (drc) {
> -if (!spapr_drc_attach(drc, dev, errp)) {
> -return;
> -}
> +/*
> + * spapr_core_pre_plug() already buys us this is a brand new
> + * core being plugged into a free slot. Nothing should already
> + * be attached to the corresponding DRC.
> + */
> +spapr_drc_attach(drc, dev, _abort);
>  
>  if (hotplugged) {
>  /*
> @@ -3981,7 +3980,7 @@ static void spapr_machine_device_plug(HotplugHandler 
> *hotplug_dev,
>  if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>  spapr_memory_plug(hotplug_dev, dev);
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> -spapr_core_plug(hotplug_dev, dev, errp);
> +spapr_core_plug(hotplug_dev, dev);
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) 
> {
>  spapr_phb_plug(hotplug_dev, dev, errp);
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_TPM_PROXY)) {

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH-for-5.2 3/4] hw/ppc/spapr_tpm_proxy: Fix hexadecimal format string specifier

[David Gibson]

On Mon, Nov 09, 2020 at 03:28:24PM +0100, Philippe Mathieu-Daudé wrote:
> Hi David,
> 
> On 11/3/20 12:39 PM, David Gibson wrote:
> > On Tue, Nov 03, 2020 at 12:25:57PM +0100, Philippe Mathieu-Daudé wrote:
> >> The '%u' conversion specifier is for decimal notation.
> >> When prefixing a format with '0x', we want the hexadecimal
> >> specifier ('%x').
> >>
> >> Inspired-by: Dov Murik 
> >> Signed-off-by: Philippe Mathieu-Daudé 
> > 
> > Acked-by: David Gibson 
> 
> As there is no qemu-trivial@ pull request during freeze/rc,
> can you queue this patch via your ppc tree?

Done, applied to ppc-for-6.0.

> 
> Thanks,
> 
> Phil.
> 
> > 
> >> ---
> >>  hw/ppc/trace-events | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> >> index dcc06d49b5a..6d8d095aa28 100644
> >> --- a/hw/ppc/trace-events
> >> +++ b/hw/ppc/trace-events
> >> @@ -19,7 +19,7 @@ spapr_update_dt_failed_size(unsigned cbold, unsigned 
> >> cbnew, unsigned magic) "Old
> >>  spapr_update_dt_failed_check(unsigned cbold, unsigned cbnew, unsigned 
> >> magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
> >>  
> >>  # spapr_tpm_proxy.c
> >> -spapr_h_tpm_comm(const char *device_path, uint64_t operation) 
> >> "tpm_device_path=%s operation=0x%"PRIu64
> >> +spapr_h_tpm_comm(const char *device_path, uint64_t operation) 
> >> "tpm_device_path=%s operation=0x%"PRIx64
> >>  spapr_tpm_execute(uint64_t data_in, uint64_t data_in_sz, uint64_t 
> >> data_out, uint64_t data_out_sz) "data_in=0x%"PRIx64", 
> >> data_in_sz=%"PRIu64", data_out=0x%"PRIx64", data_out_sz=%"PRIu64
> >>  
> >>  # spapr_iommu.c
> > 
> 
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH for-6.0 7/9] spapr: Do PHB hoplug sanity check at pre-plug

[David Gibson]

On Sat, Nov 21, 2020 at 12:42:06AM +0100, Greg Kurz wrote:
> We currently detect that a PHB index is already in use at plug time.
> But this can be decteted at pre-plug in order to error out earlier.
> 
> This allows to pass _abort to spapr_drc_attach() and to end
> up with a plug handler that doesn't need to report errors anymore.
> 
> Signed-off-by: Greg Kurz 

Applied to ppc-for-6.0, thanks.

> ---
>  hw/ppc/spapr.c | 17 +++--
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 81bac59887ab..bded059d59c8 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3872,6 +3872,7 @@ static bool spapr_phb_pre_plug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
>  SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
>  const unsigned windows_supported = spapr_phb_windows_supported(sphb);
> +SpaprDrc *drc;
>  
>  if (dev->hotplugged && !smc->dr_phb_enabled) {
>  error_setg(errp, "PHB hotplug not supported for this machine");
> @@ -3883,6 +3884,12 @@ static bool spapr_phb_pre_plug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  return false;
>  }
>  
> +drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PHB, sphb->index);
> +if (drc && drc->dev) {
> +error_setg(errp, "PHB %d already attached", sphb->index);
> +return false;
> +}
> +
>  /*
>   * This will check that sphb->index doesn't exceed the maximum number of
>   * PHBs for the current machine type.
> @@ -3896,8 +3903,7 @@ static bool spapr_phb_pre_plug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
> errp);
>  }
>  
> -static void spapr_phb_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> -   Error **errp)
> +static void spapr_phb_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
>  {
>  SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
>  SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> @@ -3913,9 +3919,8 @@ static void spapr_phb_plug(HotplugHandler *hotplug_dev, 
> DeviceState *dev,
>  /* hotplug hooks should check it's enabled before getting this far */
>  assert(drc);
>  
> -if (!spapr_drc_attach(drc, dev, errp)) {
> -return;
> -}
> +/* spapr_phb_pre_plug() already checked the DRC is attachable */
> +spapr_drc_attach(drc, dev, _abort);
>  
>  if (hotplugged) {
>  spapr_hotplug_req_add_by_index(drc);
> @@ -3983,7 +3988,7 @@ static void spapr_machine_device_plug(HotplugHandler 
> *hotplug_dev,
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
>  spapr_core_plug(hotplug_dev, dev);
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) 
> {
> -spapr_phb_plug(hotplug_dev, dev, errp);
> +spapr_phb_plug(hotplug_dev, dev);
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_TPM_PROXY)) {
>  spapr_tpm_proxy_plug(hotplug_dev, dev, errp);
>  }

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH for-6.0 9/9] spapr: spapr_drc_attach() cannot fail

[David Gibson]

On Sat, Nov 21, 2020 at 12:42:08AM +0100, Greg Kurz wrote:
> All users are passing _abort already. Document the fact
> that spapr_drc_attach() should only be passed a free DRC, which
> is supposedly the case if appropriate checking is done earlier.
> 
> Signed-off-by: Greg Kurz 

Applied to ppc-for-6.0, thanks.

> ---
>  include/hw/ppc/spapr_drc.h | 8 +++-
>  hw/ppc/spapr.c | 6 +++---
>  hw/ppc/spapr_drc.c | 8 ++--
>  hw/ppc/spapr_nvdimm.c  | 2 +-
>  hw/ppc/spapr_pci.c | 2 +-
>  5 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 165b281496bb..def3593adc8b 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -235,7 +235,13 @@ SpaprDrc *spapr_drc_by_index(uint32_t index);
>  SpaprDrc *spapr_drc_by_id(const char *type, uint32_t id);
>  int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t 
> drc_type_mask);
>  
> -bool spapr_drc_attach(SpaprDrc *drc, DeviceState *d, Error **errp);
> +/*
> + * These functions respectively abort if called with a device already
> + * attached or no device attached. In the case of spapr_drc_attach(),
> + * this means that the attachability of the DRC *must* be checked
> + * beforehand (eg. check drc->dev at pre-plug).
> + */
> +void spapr_drc_attach(SpaprDrc *drc, DeviceState *d);
>  void spapr_drc_detach(SpaprDrc *drc);
>  
>  /* Returns true if a hot plug/unplug request is pending */
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 5e32d1d396b4..e0047f41073e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3399,7 +3399,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t 
> addr_start, uint64_t size,
>   * that doesn't overlap with any existing mapping at pre-plug. The
>   * corresponding LMB DRCs are thus assumed to be all attachable.
>   */
> -spapr_drc_attach(drc, dev, _abort);
> +spapr_drc_attach(drc, dev);
>  if (!hotplugged) {
>  spapr_drc_reset(drc);
>  }
> @@ -3767,7 +3767,7 @@ static void spapr_core_plug(HotplugHandler 
> *hotplug_dev, DeviceState *dev)
>   * core being plugged into a free slot. Nothing should already
>   * be attached to the corresponding DRC.
>   */
> -spapr_drc_attach(drc, dev, _abort);
> +spapr_drc_attach(drc, dev);
>  
>  if (hotplugged) {
>  /*
> @@ -3920,7 +3920,7 @@ static void spapr_phb_plug(HotplugHandler *hotplug_dev, 
> DeviceState *dev)
>  assert(drc);
>  
>  /* spapr_phb_pre_plug() already checked the DRC is attachable */
> -spapr_drc_attach(drc, dev, _abort);
> +spapr_drc_attach(drc, dev);
>  
>  if (hotplugged) {
>  spapr_hotplug_req_add_by_index(drc);
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 77718cde1ff2..f991cf89a08a 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -369,14 +369,11 @@ static void prop_get_fdt(Object *obj, Visitor *v, const 
> char *name,
>  } while (fdt_depth != 0);
>  }
>  
> -bool spapr_drc_attach(SpaprDrc *drc, DeviceState *d, Error **errp)
> +void spapr_drc_attach(SpaprDrc *drc, DeviceState *d)
>  {
>  trace_spapr_drc_attach(spapr_drc_index(drc));
>  
> -if (drc->dev) {
> -error_setg(errp, "an attached device is still awaiting release");
> -return false;
> -}
> +g_assert(!drc->dev);
>  g_assert((drc->state == SPAPR_DRC_STATE_LOGICAL_UNUSABLE)
>   || (drc->state == SPAPR_DRC_STATE_PHYSICAL_POWERON));
>  
> @@ -386,7 +383,6 @@ bool spapr_drc_attach(SpaprDrc *drc, DeviceState *d, 
> Error **errp)
>   object_get_typename(OBJECT(drc->dev)),
>   (Object **)(>dev),
>   NULL, 0);
> -return true;
>  }
>  
>  static void spapr_drc_release(SpaprDrc *drc)
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index 2f1c196e1b76..73ee006541a6 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -101,7 +101,7 @@ void spapr_add_nvdimm(DeviceState *dev, uint64_t slot)
>   * pc_dimm_get_free_slot() provided a free slot at pre-plug. The
>   * corresponding DRC is thus assumed to be attachable.
>   */
> -spapr_drc_attach(drc, dev, _abort);
> +spapr_drc_attach(drc, dev);
>  
>  if (hotplugged) {
>  spapr_hotplug_req_add_by_index(drc);
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 2829f298d9c1..e946bd5055cc 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1601,7 +1601,7 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
>  }
>  
>  /* spapr_pci_pre_plug() already checked the DRC is attachable */
> -spapr_drc_attach(drc, DEVICE(pdev), _abort);
> +spapr_drc_attach(drc, DEVICE(pdev));
>  
>  /* If this is function 0, signal hotplug for all the device functions.
>   * Otherwise defer sending the hotplug 

Re: [RFC PATCH v2 1/5] net: Added SetSteeringEBPF method for NetClientState.

[Jason Wang]

On 2020/11/19 下午7:13, Andrew Melnychenko wrote:

From: Andrew 

For now, that method supported only by Linux TAP.
Linux TAP uses TUNSETSTEERINGEBPF ioctl.

Signed-off-by: Andrew Melnychenko 
---
  include/net/net.h |  2 ++
  net/tap-bsd.c |  5 +
  net/tap-linux.c   | 13 +
  net/tap-linux.h   |  1 +
  net/tap-solaris.c |  5 +
  net/tap-stub.c|  5 +
  net/tap.c |  9 +
  net/tap_int.h |  1 +
  8 files changed, 41 insertions(+)

diff --git a/include/net/net.h b/include/net/net.h
index 897b2d7595..d8a41fb010 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -60,6 +60,7 @@ typedef int (SetVnetBE)(NetClientState *, bool);
  typedef struct SocketReadState SocketReadState;
  typedef void (SocketReadStateFinalize)(SocketReadState *rs);
  typedef void (NetAnnounce)(NetClientState *);
+typedef bool (SetSteeringEBPF)(NetClientState *, int);
  
  typedef struct NetClientInfo {

  NetClientDriver type;
@@ -81,6 +82,7 @@ typedef struct NetClientInfo {
  SetVnetLE *set_vnet_le;
  SetVnetBE *set_vnet_be;
  NetAnnounce *announce;
+SetSteeringEBPF *set_steering_ebpf;
  } NetClientInfo;
  
  struct NetClientState {

diff --git a/net/tap-bsd.c b/net/tap-bsd.c
index 77aaf674b1..4f64f31e98 100644
--- a/net/tap-bsd.c
+++ b/net/tap-bsd.c
@@ -259,3 +259,8 @@ int tap_fd_get_ifname(int fd, char *ifname)
  {
  return -1;
  }
+
+int tap_fd_set_steering_ebpf(int fd, int prog_fd)
+{
+return -1;
+}
diff --git a/net/tap-linux.c b/net/tap-linux.c
index b0635e9e32..9584769740 100644
--- a/net/tap-linux.c
+++ b/net/tap-linux.c
@@ -316,3 +316,16 @@ int tap_fd_get_ifname(int fd, char *ifname)
  pstrcpy(ifname, sizeof(ifr.ifr_name), ifr.ifr_name);
  return 0;
  }
+
+int tap_fd_set_steering_ebpf(int fd, int prog_fd)
+{
+if (ioctl(fd, TUNSETSTEERINGEBPF, (void *) _fd) != 0) {
+error_report("Issue while setting TUNSETSTEERINGEBPF:"
+" %s with fd: %d, prog_fd: %d",
+strerror(errno), fd, prog_fd);
+
+   return -1;
+}
+
+return 0;
+}
diff --git a/net/tap-linux.h b/net/tap-linux.h
index 2f36d100fc..1d06fe0de6 100644
--- a/net/tap-linux.h
+++ b/net/tap-linux.h
@@ -31,6 +31,7 @@
  #define TUNSETQUEUE  _IOW('T', 217, int)
  #define TUNSETVNETLE _IOW('T', 220, int)
  #define TUNSETVNETBE _IOW('T', 222, int)
+#define TUNSETSTEERINGEBPF _IOR('T', 224, int)



Let's do this in another patch.

Thanks

Re: [RFC PATCH v2 0/5] eBPF RSS support for virtio-net

[Jason Wang]

On 2020/11/19 下午7:13, Andrew Melnychenko wrote:

This set of patches introduces the usage of eBPF for packet steering
and RSS hash calculation:
* RSS(Receive Side Scaling) is used to distribute network packets to
guest virtqueues by calculating packet hash
* Additionally adding support for the usage of RSS with vhost

The eBPF works on kernels 5.8+
On earlier kerneld it fails to load and the RSS feature is reported
only without vhost and implemented in 'in-qemu' software.

Implementation notes:
Linux TAP TUNSETSTEERINGEBPF ioctl was used to set the eBPF program.
Added libbpf dependency and eBPF support.
The eBPF program is part of the qemu and presented as an array
of BPF ELF file data.
The compilation of eBPF is not part of QEMU build and can be done
using provided Makefile.ebpf(need to adjust 'linuxhdrs').
Added changes to virtio-net and vhost, primary eBPF RSS is used.
'in-qemu' RSS used in the case of hash population and as a fallback option.
For vhost, the hash population feature is not reported to the guest.

Please also see the documentation in PATCH 5/5.

I am sending those patches as RFC to initiate the discussions and get
feedback on the following points:
* Fallback when eBPF is not supported by the kernel
* Live migration to the kernel that doesn't have eBPF support
* Integration with current QEMU build
* Additional usage for eBPF for packet filtering

Known issues:
* hash population not supported by eBPF RSS: 'in-qemu' RSS used
as a fallback, also, hash population feature is not reported to guests
with vhost.
* big-endian BPF support: for now, eBPF isn't supported on
big-endian systems. Can be added in future if required.
* huge .h file with eBPF binary. The size of .h file containing
eBPF binary is currently ~5K lines, because the binary is built with debug 
information.
The binary without debug/BTF info can't be loaded by libbpf.
We're looking for possibilities to reduce the size of the .h files.



A question here, is this because the binary file contains DWARF data? If 
yes, is it a building or loading dependency? If it's latter, maybe we 
can try to strip them out, anyhow it can't be recognized by kernel.


Thanks




Changes since v1:
* using libbpf instead of direct 'bpf' system call.
* added libbpf dependency to the configure/meson scripts.
* changed python script for eBPF .h file generation.
* changed eBPF program - reading L3 proto from ethernet frame.
* added TUNSETSTEERINGEBPF define for TUN.
* changed the maintainer's info.
* added license headers.
* refactored code.

Andrew (5):
   net: Added SetSteeringEBPF method for NetClientState.
   ebpf: Added eBPF RSS program.
   ebpf: Added eBPF RSS loader.
   virtio-net: Added eBPF RSS to virtio-net.
   docs: Added eBPF RSS documentation.

  MAINTAINERS|7 +
  configure  |   33 +
  docs/ebpf_rss.rst  |  133 +
  ebpf/EbpfElf_to_C.py   |   36 +
  ebpf/Makefile.ebpf |   33 +
  ebpf/ebpf_rss-stub.c   |   40 +
  ebpf/ebpf_rss.c|  186 ++
  ebpf/ebpf_rss.h|   44 +
  ebpf/meson.build   |1 +
  ebpf/rss.bpf.c |  505 +++
  ebpf/tun_rss_steering.h| 5439 
  hw/net/vhost_net.c |2 +
  hw/net/virtio-net.c|  120 +-
  include/hw/virtio/virtio-net.h |4 +
  include/net/net.h  |2 +
  meson.build|   11 +
  net/tap-bsd.c  |5 +
  net/tap-linux.c|   13 +
  net/tap-linux.h|1 +
  net/tap-solaris.c  |5 +
  net/tap-stub.c |5 +
  net/tap.c  |9 +
  net/tap_int.h  |1 +
  net/vhost-vdpa.c   |2 +
  24 files changed, 6633 insertions(+), 4 deletions(-)
  create mode 100644 docs/ebpf_rss.rst
  create mode 100644 ebpf/EbpfElf_to_C.py
  create mode 100755 ebpf/Makefile.ebpf
  create mode 100644 ebpf/ebpf_rss-stub.c
  create mode 100644 ebpf/ebpf_rss.c
  create mode 100644 ebpf/ebpf_rss.h
  create mode 100644 ebpf/meson.build
  create mode 100644 ebpf/rss.bpf.c
  create mode 100644 ebpf/tun_rss_steering.h

Re: [PATCH for-6.0 6/9] spapr: Make PHB placement functions and spapr_pre_plug_phb() return status

[David Gibson]

On Sat, Nov 21, 2020 at 12:42:05AM +0100, Greg Kurz wrote:
> Read documentation in "qapi/error.h" and changelog of commit
> e3fe3988d785 ("error: Document Error API usage rules") for
> rationale.
> 
> Signed-off-by: Greg Kurz 

Applied to ppc-for-6.0, thanks.

> ---
>  include/hw/ppc/spapr.h |  2 +-
>  hw/ppc/spapr.c | 40 +++-
>  2 files changed, 24 insertions(+), 18 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 2e89e36cfbdc..b7ced9faebf5 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -140,7 +140,7 @@ struct SpaprMachineClass {
>  bool pre_5_1_assoc_refpoints;
>  bool pre_5_2_numa_associativity;
>  
> -void (*phb_placement)(SpaprMachineState *spapr, uint32_t index,
> +bool (*phb_placement)(SpaprMachineState *spapr, uint32_t index,
>uint64_t *buid, hwaddr *pio, 
>hwaddr *mmio32, hwaddr *mmio64,
>unsigned n_dma, uint32_t *liobns, hwaddr *nv2gpa,
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index cfca033c7b14..81bac59887ab 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3865,7 +3865,7 @@ int spapr_phb_dt_populate(SpaprDrc *drc, 
> SpaprMachineState *spapr,
>  return 0;
>  }
>  
> -static void spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +static bool spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> Error **errp)
>  {
>  SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
> @@ -3875,24 +3875,25 @@ static void spapr_phb_pre_plug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  
>  if (dev->hotplugged && !smc->dr_phb_enabled) {
>  error_setg(errp, "PHB hotplug not supported for this machine");
> -return;
> +return false;
>  }
>  
>  if (sphb->index == (uint32_t)-1) {
>  error_setg(errp, "\"index\" for PAPR PHB is mandatory");
> -return;
> +return false;
>  }
>  
>  /*
>   * This will check that sphb->index doesn't exceed the maximum number of
>   * PHBs for the current machine type.
>   */
> -smc->phb_placement(spapr, sphb->index,
> -   >buid, >io_win_addr,
> -   >mem_win_addr, >mem64_win_addr,
> -   windows_supported, sphb->dma_liobn,
> -   >nv2_gpa_win_addr, >nv2_atsd_win_addr,
> -   errp);
> +return
> +smc->phb_placement(spapr, sphb->index,
> +   >buid, >io_win_addr,
> +   >mem_win_addr, >mem64_win_addr,
> +   windows_supported, sphb->dma_liobn,
> +   >nv2_gpa_win_addr, >nv2_atsd_win_addr,
> +   errp);
>  }
>  
>  static void spapr_phb_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> @@ -4130,7 +4131,7 @@ static const CPUArchIdList 
> *spapr_possible_cpu_arch_ids(MachineState *machine)
>  return machine->possible_cpus;
>  }
>  
> -static void spapr_phb_placement(SpaprMachineState *spapr, uint32_t index,
> +static bool spapr_phb_placement(SpaprMachineState *spapr, uint32_t index,
>  uint64_t *buid, hwaddr *pio,
>  hwaddr *mmio32, hwaddr *mmio64,
>  unsigned n_dma, uint32_t *liobns,
> @@ -4168,7 +4169,7 @@ static void spapr_phb_placement(SpaprMachineState 
> *spapr, uint32_t index,
>  if (index >= SPAPR_MAX_PHBS) {
>  error_setg(errp, "\"index\" for PAPR PHB is too large (max %llu)",
> SPAPR_MAX_PHBS - 1);
> -return;
> +return false;
>  }
>  
>  *buid = base_buid + index;
> @@ -4182,6 +4183,7 @@ static void spapr_phb_placement(SpaprMachineState 
> *spapr, uint32_t index,
>  
>  *nv2gpa = SPAPR_PCI_NV2RAM64_WIN_BASE + index * 
> SPAPR_PCI_NV2RAM64_WIN_SIZE;
>  *nv2atsd = SPAPR_PCI_NV2ATSD_WIN_BASE + index * 
> SPAPR_PCI_NV2ATSD_WIN_SIZE;
> +return true;
>  }
>  
>  static ICSState *spapr_ics_get(XICSFabric *dev, int irq)
> @@ -4561,18 +4563,21 @@ DEFINE_SPAPR_MACHINE(4_1, "4.1", false);
>  /*
>   * pseries-4.0
>   */
> -static void phb_placement_4_0(SpaprMachineState *spapr, uint32_t index,
> +static bool phb_placement_4_0(SpaprMachineState *spapr, uint32_t index,
>uint64_t *buid, hwaddr *pio,
>hwaddr *mmio32, hwaddr *mmio64,
>unsigned n_dma, uint32_t *liobns,
>hwaddr *nv2gpa, hwaddr *nv2atsd, Error **errp)
>  {
> -spapr_phb_placement(spapr, index, buid, pio, mmio32, mmio64, n_dma, 
> liobns,
> -nv2gpa, nv2atsd, errp);
> +if (!spapr_phb_placement(spapr, index, buid, pio, mmio32, mmio64, n_dma,
> + liobns, nv2gpa, nv2atsd, errp)) {

Re: [PATCH for-6.0 8/9] spapr: Do TPM proxy hotplug sanity checks at pre-plug

[David Gibson]

On Sat, Nov 21, 2020 at 12:42:07AM +0100, Greg Kurz wrote:
> There can be only one TPM proxy at a time. This is currently
> checked at plug time. But this can be detected at pre-plug in
> order to error out earlier.
> 
> This allows to get rid of error handling in the plug handler.
> 
> Signed-off-by: Greg Kurz 

Applied to ppc-for-6.0, thanks.

> ---
>  hw/ppc/spapr.c | 23 ++-
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index bded059d59c8..5e32d1d396b4 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3957,17 +3957,28 @@ static void spapr_phb_unplug_request(HotplugHandler 
> *hotplug_dev,
>  }
>  }
>  
> -static void spapr_tpm_proxy_plug(HotplugHandler *hotplug_dev, DeviceState 
> *dev,
> - Error **errp)
> +static
> +bool spapr_tpm_proxy_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +  Error **errp)
>  {
>  SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
> -SpaprTpmProxy *tpm_proxy = SPAPR_TPM_PROXY(dev);
>  
>  if (spapr->tpm_proxy != NULL) {
>  error_setg(errp, "Only one TPM proxy can be specified for this 
> machine");
> -return;
> +return false;
>  }
>  
> +return true;
> +}
> +
> +static void spapr_tpm_proxy_plug(HotplugHandler *hotplug_dev, DeviceState 
> *dev)
> +{
> +SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
> +SpaprTpmProxy *tpm_proxy = SPAPR_TPM_PROXY(dev);
> +
> +/* Already checked in spapr_tpm_proxy_pre_plug() */
> +g_assert(spapr->tpm_proxy == NULL);
> +
>  spapr->tpm_proxy = tpm_proxy;
>  }
>  
> @@ -3990,7 +4001,7 @@ static void spapr_machine_device_plug(HotplugHandler 
> *hotplug_dev,
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) 
> {
>  spapr_phb_plug(hotplug_dev, dev);
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_TPM_PROXY)) {
> -spapr_tpm_proxy_plug(hotplug_dev, dev, errp);
> +spapr_tpm_proxy_plug(hotplug_dev, dev);
>  }
>  }
>  
> @@ -4053,6 +4064,8 @@ static void 
> spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
>  spapr_core_pre_plug(hotplug_dev, dev, errp);
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) 
> {
>  spapr_phb_pre_plug(hotplug_dev, dev, errp);
> +} else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_TPM_PROXY)) {
> +spapr_tpm_proxy_pre_plug(hotplug_dev, dev, errp);
>  }
>  }
>  

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH for-6.0 4/9] spapr: Set compat mode in spapr_reset_vcpu()

[David Gibson]

On Sat, Nov 21, 2020 at 12:42:03AM +0100, Greg Kurz wrote:
> When it comes to resetting the compat mode of the vCPUS, there are
> two situations to consider:
> (1) machine reset should set the compat mode back to the machine default,
> ie. spapr->max_compat_pvr
> (2) hot plugged vCPUs should set their compat mode to mach the boot vCPU,
> ie. POWERPC_CPU(first_cpu)->compat_pvr
> 
> This is currently handled in two separate places: globally for all vCPUs
> from the machine reset code for (1) and for each thread of a core from
> the hotplug path for (2).
> 
> Since the machine reset code already resets all vCPUs, starting with boot
> vCPU, consolidate the logic in spapr_reset_vcpu(). Special case the boot
> vCPU so that it resets its compat mode back to the machine default. Any
> other vCPU just need to match the compat mode of the boot vCPU.
> 
> Failing to set the compat mode during machine reset is a fatal error,
> but not for hot plugged vCPUs. This is arguable because if we've been
> able to set the boot vCPU compat mode at CAS or during machine reset,
> it should definitely not fail for other vCPUs. Since spapr_reset_vcpu()
> already has a fatal error path for kvm_check_mmu() failures, do the
> same for ppc_set_compat().
> 
> This gets rid of an error path in spapr_core_plug(). It will allow
> further simplifications.
> 
> Signed-off-by: Greg Kurz 
> ---
>  hw/ppc/spapr.c  | 16 
>  hw/ppc/spapr_cpu_core.c | 13 +
>  2 files changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f58f77389e8e..da7586f548df 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1606,8 +1606,6 @@ static void spapr_machine_reset(MachineState *machine)
>  spapr_ovec_cleanup(spapr->ov5_cas);
>  spapr->ov5_cas = spapr_ovec_new();
>  
> -ppc_set_compat_all(spapr->max_compat_pvr, _fatal);
> -
>  /*
>   * This is fixing some of the default configuration of the XIVE
>   * devices. To be called after the reset of the machine devices.
> @@ -3785,20 +3783,6 @@ static void spapr_core_plug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  
>  core_slot->cpu = OBJECT(dev);
>  
> -/*
> - * Set compatibility mode to match the boot CPU, which was either set
> - * by the machine reset code or by CAS.
> - */
> -if (hotplugged) {
> -for (i = 0; i < cc->nr_threads; i++) {
> -if (ppc_set_compat(core->threads[i],
> -   POWERPC_CPU(first_cpu)->compat_pvr,
> -   errp) < 0) {
> -return;
> -}
> -}
> -}
> -
>  if (smc->pre_2_10_has_unused_icps) {
>  for (i = 0; i < cc->nr_threads; i++) {
>  cs = CPU(core->threads[i]);
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 2f7dc3c23ded..17741a3fb77f 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -27,6 +27,7 @@
>  
>  static void spapr_reset_vcpu(PowerPCCPU *cpu)
>  {
> +PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
>  CPUState *cs = CPU(cpu);
>  CPUPPCState *env = >env;
>  PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> @@ -69,6 +70,18 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
>  kvm_check_mmu(cpu, _fatal);
>  
>  spapr_irq_cpu_intc_reset(spapr, cpu);
> +
> +/*
> + * The boot CPU is only reset during machine reset : reset its
> + * compatibility mode to the machine default. For other CPUs,
> + * either cold plugged or hot plugged, set the compatibility mode
> + * to match the boot CPU, which was either set by the machine reset
> + * code or by CAS.
> + */
> +ppc_set_compat(cpu,
> +   cpu == first_ppc_cpu ?
> +   spapr->max_compat_pvr : first_ppc_cpu->compat_pvr,
> +   _fatal);

This assumes that when it is called for a non-boot CPU, it has already
been called for the boot CPU..  Are we certain that's guaranteed by
the sequence of reset calls during a full machine reset?

>  }
>  
>  void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip,

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH for-6.0 2/9] spapr: Do NVDIMM/PC-DIMM device hotplug sanity checks at pre-plug only

[David Gibson]

On Sat, Nov 21, 2020 at 12:42:01AM +0100, Greg Kurz wrote:
> Pre-plug of a memory device, be it an NVDIMM or a PC-DIMM, ensures
> that the memory slot is available and that addresses don't overlap
> with existing memory regions. The corresponding DRCs in the LMB
> and PMEM namespaces are thus necessarily attachable at plug time.
> 
> Pass _abort to spapr_drc_attach() in spapr_add_lmbs() and
> spapr_add_nvdimm(). This allows to greatly simplify error handling
> on the plug path.
> 
> Signed-off-by: Greg Kurz 

Applied to ppc-for-6.0, thanks.

> ---
>  include/hw/ppc/spapr_nvdimm.h |  2 +-
>  hw/ppc/spapr.c| 40 ---
>  hw/ppc/spapr_nvdimm.c | 11 +-
>  3 files changed, 20 insertions(+), 33 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h
> index 344582d2f5f7..73be250e2ac9 100644
> --- a/include/hw/ppc/spapr_nvdimm.h
> +++ b/include/hw/ppc/spapr_nvdimm.h
> @@ -30,6 +30,6 @@ int spapr_pmem_dt_populate(SpaprDrc *drc, SpaprMachineState 
> *spapr,
>  void spapr_dt_persistent_memory(SpaprMachineState *spapr, void *fdt);
>  bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
> uint64_t size, Error **errp);
> -bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp);
> +void spapr_add_nvdimm(DeviceState *dev, uint64_t slot);
>  
>  #endif
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 12a012d9dd09..394d28d9e081 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3382,8 +3382,8 @@ int spapr_lmb_dt_populate(SpaprDrc *drc, 
> SpaprMachineState *spapr,
>  return 0;
>  }
>  
> -static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t 
> size,
> -   bool dedicated_hp_event_source, Error **errp)
> +static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t 
> size,
> +   bool dedicated_hp_event_source)
>  {
>  SpaprDrc *drc;
>  uint32_t nr_lmbs = size/SPAPR_MEMORY_BLOCK_SIZE;
> @@ -3396,15 +3396,12 @@ static bool spapr_add_lmbs(DeviceState *dev, uint64_t 
> addr_start, uint64_t size,
>addr / SPAPR_MEMORY_BLOCK_SIZE);
>  g_assert(drc);
>  
> -if (!spapr_drc_attach(drc, dev, errp)) {
> -while (addr > addr_start) {
> -addr -= SPAPR_MEMORY_BLOCK_SIZE;
> -drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
> -  addr / SPAPR_MEMORY_BLOCK_SIZE);
> -spapr_drc_detach(drc);
> -}
> -return false;
> -}
> +/*
> + * memory_device_get_free_addr() provided a range of free addresses
> + * that doesn't overlap with any existing mapping at pre-plug. The
> + * corresponding LMB DRCs are thus assumed to be all attachable.
> + */
> +spapr_drc_attach(drc, dev, _abort);
>  if (!hotplugged) {
>  spapr_drc_reset(drc);
>  }
> @@ -3425,11 +3422,9 @@ static bool spapr_add_lmbs(DeviceState *dev, uint64_t 
> addr_start, uint64_t size,
> nr_lmbs);
>  }
>  }
> -return true;
>  }
>  
> -static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> -  Error **errp)
> +static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
>  {
>  SpaprMachineState *ms = SPAPR_MACHINE(hotplug_dev);
>  PCDIMMDevice *dimm = PC_DIMM(dev);
> @@ -3444,24 +3439,15 @@ static void spapr_memory_plug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  if (!is_nvdimm) {
>  addr = object_property_get_uint(OBJECT(dimm),
>  PC_DIMM_ADDR_PROP, _abort);
> -if (!spapr_add_lmbs(dev, addr, size,
> -spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), errp)) 
> {
> -goto out_unplug;
> -}
> +spapr_add_lmbs(dev, addr, size,
> +   spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT));
>  } else {
>  slot = object_property_get_int(OBJECT(dimm),
> PC_DIMM_SLOT_PROP, _abort);
>  /* We should have valid slot number at this point */
>  g_assert(slot >= 0);
> -if (!spapr_add_nvdimm(dev, slot, errp)) {
> -goto out_unplug;
> -}
> +spapr_add_nvdimm(dev, slot);
>  }
> -
> -return;
> -
> -out_unplug:
> -pc_dimm_unplug(dimm, MACHINE(ms));
>  }
>  
>  static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState 
> *dev,
> @@ -4009,7 +3995,7 @@ static void spapr_machine_device_plug(HotplugHandler 
> *hotplug_dev,
>DeviceState *dev, Error **errp)
>  {
>  if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -spapr_memory_plug(hotplug_dev, dev, errp);
> +

Re: [PATCH for-6.0 1/9] spapr: Do PCI device hotplug sanity checks at pre-plug only

[David Gibson]

On Sat, Nov 21, 2020 at 12:42:00AM +0100, Greg Kurz wrote:
> The PHB acts as the hotplug handler for PCI devices. It does some
> sanity checks on DR enablement, PCI bridge chassis numbers and
> multifunction. These checks are currently performed at plug time,
> but they would best sit in a pre-plug handler in order to error
> out as early as possible.
> 
> Create a spapr_pci_pre_plug() handler and move all the checking
> there. Add a check that the associated DRC doesn't already have
> an attached device. This is equivalent to the slot availability
> check performed by do_pci_register_device() upon realization of
> the PCI device.
> 
> This allows to pass _abort to spapr_drc_attach() and to end
> up with a plug handler that doesn't need to report errors anymore.
> 
> Signed-off-by: Greg Kurz 

Applied to ppc-for-6.0, thanks.

> ---
>  hw/ppc/spapr_pci.c | 43 +--
>  1 file changed, 33 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 88ce87f130a5..2829f298d9c1 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1532,8 +1532,8 @@ static bool bridge_has_valid_chassis_nr(Object *bridge, 
> Error **errp)
>  return true;
>  }
>  
> -static void spapr_pci_plug(HotplugHandler *plug_handler,
> -   DeviceState *plugged_dev, Error **errp)
> +static void spapr_pci_pre_plug(HotplugHandler *plug_handler,
> +   DeviceState *plugged_dev, Error **errp)
>  {
>  SpaprPhbState *phb = SPAPR_PCI_HOST_BRIDGE(DEVICE(plug_handler));
>  PCIDevice *pdev = PCI_DEVICE(plugged_dev);
> @@ -1542,9 +1542,6 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
>  PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(pdev)));
>  uint32_t slotnr = PCI_SLOT(pdev->devfn);
>  
> -/* if DR is disabled we don't need to do anything in the case of
> - * hotplug or coldplug callbacks
> - */
>  if (!phb->dr_enabled) {
>  /* if this is a hotplug operation initiated by the user
>   * we need to let them know it's not enabled
> @@ -1552,17 +1549,14 @@ static void spapr_pci_plug(HotplugHandler 
> *plug_handler,
>  if (plugged_dev->hotplugged) {
>  error_setg(errp, QERR_BUS_NO_HOTPLUG,
> object_get_typename(OBJECT(phb)));
> +return;
>  }
> -return;
>  }
>  
> -g_assert(drc);
> -
>  if (pc->is_bridge) {
>  if (!bridge_has_valid_chassis_nr(OBJECT(plugged_dev), errp)) {
>  return;
>  }
> -spapr_pci_bridge_plug(phb, PCI_BRIDGE(plugged_dev));
>  }
>  
>  /* Following the QEMU convention used for PCIe multifunction
> @@ -1574,13 +1568,41 @@ static void spapr_pci_plug(HotplugHandler 
> *plug_handler,
>  error_setg(errp, "PCI: slot %d function 0 already occupied by %s,"
> " additional functions can no longer be exposed to 
> guest.",
> slotnr, bus->devices[PCI_DEVFN(slotnr, 0)]->name);
> +}
> +
> +if (drc && drc->dev) {
> +error_setg(errp, "PCI: slot %d already occupied by %s", slotnr,
> +   pci_get_function_0(PCI_DEVICE(drc->dev))->name);
>  return;
>  }
> +}
> +
> +static void spapr_pci_plug(HotplugHandler *plug_handler,
> +   DeviceState *plugged_dev, Error **errp)
> +{
> +SpaprPhbState *phb = SPAPR_PCI_HOST_BRIDGE(DEVICE(plug_handler));
> +PCIDevice *pdev = PCI_DEVICE(plugged_dev);
> +PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(plugged_dev);
> +SpaprDrc *drc = drc_from_dev(phb, pdev);
> +uint32_t slotnr = PCI_SLOT(pdev->devfn);
>  
> -if (!spapr_drc_attach(drc, DEVICE(pdev), errp)) {
> +/*
> + * If DR is disabled we don't need to do anything in the case of
> + * hotplug or coldplug callbacks.
> + */
> +if (!phb->dr_enabled) {
>  return;
>  }
>  
> +g_assert(drc);
> +
> +if (pc->is_bridge) {
> +spapr_pci_bridge_plug(phb, PCI_BRIDGE(plugged_dev));
> +}
> +
> +/* spapr_pci_pre_plug() already checked the DRC is attachable */
> +spapr_drc_attach(drc, DEVICE(pdev), _abort);
> +
>  /* If this is function 0, signal hotplug for all the device functions.
>   * Otherwise defer sending the hotplug event.
>   */
> @@ -2223,6 +2245,7 @@ static void spapr_phb_class_init(ObjectClass *klass, 
> void *data)
>  /* Supported by TYPE_SPAPR_MACHINE */
>  dc->user_creatable = true;
>  set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> +hp->pre_plug = spapr_pci_pre_plug;
>  hp->plug = spapr_pci_plug;
>  hp->unplug = spapr_pci_unplug;
>  hp->unplug_request = spapr_pci_unplug_request;

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH for-6.0 3/9] spapr: Fix pre-2.10 dummy ICP hack

[David Gibson]

On Sat, Nov 21, 2020 at 12:42:02AM +0100, Greg Kurz wrote:
> This hack registers dummy VMState entries of ICPs in order to
> support migration of old pseries machine types that used to
> create all smp.max_cpus possible ICPs at machine init.
> 
> Part of the work is to unregister the dummy entries when plugging
> an actual vCPU core, and to register them back when unplugging the
> core. The code that unregisters the dummy ICPs in spapr_core_plug()
> is misplaced: if ppc_set_compat() fails afterwards, the hotplug
> operation will be cancelled and the dummy ICPs won't be registered
> back since the unplug handler isn't called.
> 
> Unregister the dummy ICPs at the end of spapr_core_plug().
> 
> Signed-off-by: Greg Kurz 

Applied to ppc-for-6.0, thanks.

> ---
> 
> The next patch makes this patch a bit useless. I post it
> anyway for the records because it is a programming error.
> ---
>  hw/ppc/spapr.c | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 394d28d9e081..f58f77389e8e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3785,13 +3785,6 @@ static void spapr_core_plug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  
>  core_slot->cpu = OBJECT(dev);
>  
> -if (smc->pre_2_10_has_unused_icps) {
> -for (i = 0; i < cc->nr_threads; i++) {
> -cs = CPU(core->threads[i]);
> -pre_2_10_vmstate_unregister_dummy_icp(cs->cpu_index);
> -}
> -}
> -
>  /*
>   * Set compatibility mode to match the boot CPU, which was either set
>   * by the machine reset code or by CAS.
> @@ -3805,6 +3798,13 @@ static void spapr_core_plug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  }
>  }
>  }
> +
> +if (smc->pre_2_10_has_unused_icps) {
> +for (i = 0; i < cc->nr_threads; i++) {
> +cs = CPU(core->threads[i]);
> +pre_2_10_vmstate_unregister_dummy_icp(cs->cpu_index);
> +}
> +}
>  }
>  
>  static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState 
> *dev,

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH for-6.0 5/8] spapr/xics: Drop unused argument to xics_kvm_has_broken_disconnect()

[David Gibson]

On Fri, Nov 20, 2020 at 06:46:43PM +0100, Greg Kurz wrote:
> Never used from the start.
> 
> Signed-off-by: Greg Kurz 

Applied to ppc-for-6.0, thanks.

> ---
>  include/hw/ppc/xics_spapr.h | 2 +-
>  hw/intc/xics_kvm.c  | 2 +-
>  hw/ppc/spapr_irq.c  | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h
> index 0b8182e40b33..de752c0d2c7e 100644
> --- a/include/hw/ppc/xics_spapr.h
> +++ b/include/hw/ppc/xics_spapr.h
> @@ -38,6 +38,6 @@ DECLARE_INSTANCE_CHECKER(ICSState, ICS_SPAPR,
>  int xics_kvm_connect(SpaprInterruptController *intc, uint32_t nr_servers,
>   Error **errp);
>  void xics_kvm_disconnect(SpaprInterruptController *intc);
> -bool xics_kvm_has_broken_disconnect(SpaprMachineState *spapr);
> +bool xics_kvm_has_broken_disconnect(void);
>  
>  #endif /* XICS_SPAPR_H */
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 68bb1914b9bb..570d635bcc08 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -484,7 +484,7 @@ void xics_kvm_disconnect(SpaprInterruptController *intc)
>   * support destruction of a KVM XICS device while the VM is running.
>   * Required to start a spapr machine with ic-mode=dual,kernel-irqchip=on.
>   */
> -bool xics_kvm_has_broken_disconnect(SpaprMachineState *spapr)
> +bool xics_kvm_has_broken_disconnect(void)
>  {
>  int rc;
>  
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index a0fc474ecb06..2dacbecfd5fd 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -186,7 +186,7 @@ static int spapr_irq_check(SpaprMachineState *spapr, 
> Error **errp)
>  if (kvm_enabled() &&
>  spapr->irq == _irq_dual &&
>  kvm_kernel_irqchip_required() &&
> -xics_kvm_has_broken_disconnect(spapr)) {
> +xics_kvm_has_broken_disconnect()) {
>  error_setg(errp,
>  "KVM is incompatible with ic-mode=dual,kernel-irqchip=on");
>  error_append_hint(errp,

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH for-6.0 6/8] spapr/xics: Add "nr-servers" property

[David Gibson]

On Fri, Nov 20, 2020 at 06:46:44PM +0100, Greg Kurz wrote:
> The sPAPR ICS device exposes the range of vCPU ids it can handle in
> the "ibm,interrupt-server-ranges" FDT property. The highest vCPU
> id, ie. spapr_max_server_number(), is obtained from the machine
> through the "nr_servers" argument of the generic spapr_irq_dt() call.
> 
> We want to drop the "nr_servers" argument from the API because it
> doesn't make sense for the sPAPR XIVE device actually.
> 
> On POWER9, we also pass the highest vCPU id to the KVM XICS-on-XIVE
> device, in order to optimize resource allocation in the HW.
> 
> This is enough motivation to introduce an "nr-servers" property
> and to use it for both purposes.
> 
> Signed-off-by: Greg Kurz 
> ---
>  include/hw/ppc/spapr.h  |  4 ++--
>  include/hw/ppc/xics_spapr.h | 21 +---
>  hw/intc/xics_kvm.c  |  2 +-
>  hw/intc/xics_spapr.c| 38 -
>  hw/ppc/spapr.c  |  5 +++--
>  hw/ppc/spapr_irq.c  |  7 +--
>  6 files changed, 54 insertions(+), 23 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 2e89e36cfbdc..b76c84a2f884 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -10,7 +10,7 @@
>  #include "hw/ppc/spapr_irq.h"
>  #include "qom/object.h"
>  #include "hw/ppc/spapr_xive.h"  /* For SpaprXive */
> -#include "hw/ppc/xics.h"/* For ICSState */
> +#include "hw/ppc/xics_spapr.h"  /* For IcsSpaprState */
>  #include "hw/ppc/spapr_tpm_proxy.h"
>  
>  struct SpaprVioBus;
> @@ -230,7 +230,7 @@ struct SpaprMachineState {
>  SpaprIrq *irq;
>  qemu_irq *qirqs;
>  SpaprInterruptController *active_intc;
> -ICSState *ics;
> +IcsSpaprState *ics;
>  SpaprXive *xive;
>  
>  bool cmd_line_caps[SPAPR_CAP_NUM];
> diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h
> index de752c0d2c7e..1a483a873b62 100644
> --- a/include/hw/ppc/xics_spapr.h
> +++ b/include/hw/ppc/xics_spapr.h
> @@ -28,12 +28,27 @@
>  #define XICS_SPAPR_H
>  
>  #include "hw/ppc/spapr.h"
> +#include "hw/ppc/xics.h"
>  #include "qom/object.h"
>  
> +typedef struct IcsSpaprState {
> +/*< private >*/
> +ICPState parent_obj;

It's called "*Ics*SpaprState" and it's replacing an ICSState, but it's
parent object is an *ICP*State - that doesn't seem right.

> +
> +/*
> + * The ICS needs to know the upper limit to vCPU ids it
> + * might be exposed to in order to size the vCPU id range
> + * in "ibm,interrupt-server-ranges" and to optimize HW
> + * resource allocation when using the XICS-on-XIVE KVM
> + * device. It is the purpose of the "nr-servers" property
> + * which *must* be set to a non-null value before realizing
> + * the ICS.
> + */
> +uint32_t nr_servers;

That said, I'm a but dubious about attaching the number of servers to
the ICS side, rather than the ICP side, since server numbers is
basically an ICP concept.  In fact... things about the overall
topology are usually done via the XicsFabric, so I'm wondering if we
should add a callback there for nr_servers.

> +} IcsSpaprState;
> +
>  #define TYPE_ICS_SPAPR "ics-spapr"
> -/* This is reusing the ICSState typedef from TYPE_ICS */
> -DECLARE_INSTANCE_CHECKER(ICSState, ICS_SPAPR,
> - TYPE_ICS_SPAPR)
> +DECLARE_INSTANCE_CHECKER(IcsSpaprState, ICS_SPAPR, TYPE_ICS_SPAPR)
>  
>  int xics_kvm_connect(SpaprInterruptController *intc, uint32_t nr_servers,
>   Error **errp);
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 570d635bcc08..ecbbda0e249b 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -350,7 +350,7 @@ void ics_kvm_set_irq(ICSState *ics, int srcno, int val)
>  int xics_kvm_connect(SpaprInterruptController *intc, uint32_t nr_servers,
>   Error **errp)
>  {
> -ICSState *ics = ICS_SPAPR(intc);
> +ICSState *ics = ICS(intc);
>  int rc;
>  CPUState *cs;
>  Error *local_err = NULL;
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 8ae4f41459c3..ce147e8980ed 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -34,6 +34,7 @@
>  #include "hw/ppc/xics.h"
>  #include "hw/ppc/xics_spapr.h"
>  #include "hw/ppc/fdt.h"
> +#include "hw/qdev-properties.h"
>  #include "qapi/visitor.h"
>  
>  /*
> @@ -154,7 +155,7 @@ static void rtas_set_xive(PowerPCCPU *cpu, 
> SpaprMachineState *spapr,
>uint32_t nargs, target_ulong args,
>uint32_t nret, target_ulong rets)
>  {
> -ICSState *ics = spapr->ics;
> +ICSState *ics = ICS(spapr->ics);
>  uint32_t nr, srcno, server, priority;
>  
>  CHECK_EMULATED_XICS_RTAS(spapr, rets);
> @@ -189,7 +190,7 @@ static void rtas_get_xive(PowerPCCPU *cpu, 
> SpaprMachineState *spapr,
>uint32_t nargs, target_ulong args,
>uint32_t nret, target_ulong rets)
> 

Re: [PATCH for-6.0 1/8] spapr/xive: Turn some sanity checks into assertions

[David Gibson]

On Fri, Nov 20, 2020 at 06:46:39PM +0100, Greg Kurz wrote:
> The sPAPR XIVE device is created by the machine in spapr_irq_init().
> The latter overrides any value provided by the user with -global for
> the "nr-irqs" and "nr-ends" properties with strictly positive values.
> 
> It seems reasonable to assume these properties should never be 0,
> which wouldn't make much sense by the way.
> 
> Signed-off-by: Greg Kurz 

Applied to ppc-for-6.0.

> ---
>  hw/intc/spapr_xive.c | 14 --
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index 1fa09f287ac0..60e0d5769dcc 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -296,22 +296,16 @@ static void spapr_xive_realize(DeviceState *dev, Error 
> **errp)
>  XiveENDSource *end_xsrc = >end_source;
>  Error *local_err = NULL;
>  
> +/* Set by spapr_irq_init() */
> +g_assert(xive->nr_irqs);
> +g_assert(xive->nr_ends);
> +
>  sxc->parent_realize(dev, _err);
>  if (local_err) {
>  error_propagate(errp, local_err);
>  return;
>  }
>  
> -if (!xive->nr_irqs) {
> -error_setg(errp, "Number of interrupt needs to be greater 0");
> -return;
> -}
> -
> -if (!xive->nr_ends) {
> -error_setg(errp, "Number of interrupt needs to be greater 0");
> -return;
> -}
> -
>  /*
>   * Initialize the internal sources, for IPIs and virtual devices.
>   */

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH for-6.0 7/8] spapr: Drop "nr_servers" argument of the sPAPR IC activate() operation

[David Gibson]

On Fri, Nov 20, 2020 at 06:46:45PM +0100, Greg Kurz wrote:
> This argument isn't used by the backends anymore.
> 
> Signed-off-by: Greg Kurz 
> ---
>  include/hw/ppc/spapr_irq.h | 3 +--
>  hw/intc/spapr_xive.c   | 3 +--
>  hw/intc/xics_spapr.c   | 3 +--
>  hw/ppc/spapr_irq.c | 3 +--
>  4 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index c22a72c9e270..3e1c619d4c06 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -43,8 +43,7 @@ DECLARE_CLASS_CHECKERS(SpaprInterruptControllerClass, 
> SPAPR_INTC,
>  struct SpaprInterruptControllerClass {
>  InterfaceClass parent;
>  
> -int (*activate)(SpaprInterruptController *intc, uint32_t nr_servers,
> -Error **errp);

Hm.  Thinking back on this, is the problem just that it's not clear
here if the 'nr_servers' parameter here indicates the number of CPU
targets, or the maximum index of CPU targets?

If so, I wonder if it might be better to pass both numbers as
parameters here, rather than shuffling the properties of the devices
themselves.

> +int (*activate)(SpaprInterruptController *intc, Error **errp);
>  void (*deactivate)(SpaprInterruptController *intc);
>  
>  /*
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index d13a2955ce9b..8331026fdb12 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -754,8 +754,7 @@ static void spapr_xive_dt(SpaprInterruptController *intc, 
> uint32_t nr_servers,
>   plat_res_int_priorities, 
> sizeof(plat_res_int_priorities)));
>  }
>  
> -static int spapr_xive_activate(SpaprInterruptController *intc,
> -   uint32_t nr_servers, Error **errp)
> +static int spapr_xive_activate(SpaprInterruptController *intc, Error **errp)
>  {
>  SpaprXive *xive = SPAPR_XIVE(intc);
>  
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index ce147e8980ed..8810bd93c856 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -426,8 +426,7 @@ static int xics_spapr_post_load(SpaprInterruptController 
> *intc, int version_id)
>  return 0;
>  }
>  
> -static int xics_spapr_activate(SpaprInterruptController *intc,
> -   uint32_t nr_servers, Error **errp)
> +static int xics_spapr_activate(SpaprInterruptController *intc, Error **errp)
>  {
>  if (kvm_enabled()) {
>  return spapr_irq_init_kvm(xics_kvm_connect, intc,
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index be6f4041e433..f2897fbc942a 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -480,7 +480,6 @@ static void set_active_intc(SpaprMachineState *spapr,
>  SpaprInterruptController *new_intc)
>  {
>  SpaprInterruptControllerClass *sicc;
> -uint32_t nr_servers = spapr_max_server_number(spapr);
>  
>  assert(new_intc);
>  
> @@ -498,7 +497,7 @@ static void set_active_intc(SpaprMachineState *spapr,
>  
>  sicc = SPAPR_INTC_GET_CLASS(new_intc);
>  if (sicc->activate) {
> -sicc->activate(new_intc, nr_servers, _fatal);
> +sicc->activate(new_intc, _fatal);
>  }
>  
>  spapr->active_intc = new_intc;

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH for-6.0 2/8] spapr/xive: Introduce spapr_xive_nr_ends()

[David Gibson]

On Fri, Nov 20, 2020 at 06:46:40PM +0100, Greg Kurz wrote:
> We're going to kill the "nr_ends" field in a subsequent patch.
> Prepare ground by using an helper instead of peeking into
> the sPAPR XIVE structure directly.
> 
> Signed-off-by: Greg Kurz 

Applied to ppc-for-6.0, thanks.


> ---
>  include/hw/ppc/spapr_xive.h |  1 +
>  hw/intc/spapr_xive.c| 23 ++-
>  hw/intc/spapr_xive_kvm.c|  4 ++--
>  3 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index 26c8d90d7196..4b967f13c030 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -75,6 +75,7 @@ void spapr_xive_map_mmio(SpaprXive *xive);
>  
>  int spapr_xive_end_to_target(uint8_t end_blk, uint32_t end_idx,
>   uint32_t *out_server, uint8_t *out_prio);
> +uint32_t spapr_xive_nr_ends(const SpaprXive *xive);
>  
>  /*
>   * KVM XIVE device helpers
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index 60e0d5769dcc..f473ad9cba47 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -192,7 +192,7 @@ void spapr_xive_pic_print_info(SpaprXive *xive, Monitor 
> *mon)
>  uint32_t end_idx = xive_get_field64(EAS_END_INDEX, eas->w);
>  XiveEND *end;
>  
> -assert(end_idx < xive->nr_ends);
> +assert(end_idx < spapr_xive_nr_ends(xive));
>  end = >endt[end_idx];
>  
>  if (xive_end_is_valid(end)) {
> @@ -270,7 +270,7 @@ static void spapr_xive_reset(void *dev)
>  }
>  
>  /* Clear all ENDs */
> -for (i = 0; i < xive->nr_ends; i++) {
> +for (i = 0; i < spapr_xive_nr_ends(xive); i++) {
>  spapr_xive_end_reset(>endt[i]);
>  }
>  }
> @@ -288,6 +288,11 @@ static void spapr_xive_instance_init(Object *obj)
>  xive->fd = -1;
>  }
>  
> +uint32_t spapr_xive_nr_ends(const SpaprXive *xive)
> +{
> +return xive->nr_ends;
> +}
> +
>  static void spapr_xive_realize(DeviceState *dev, Error **errp)
>  {
>  SpaprXive *xive = SPAPR_XIVE(dev);
> @@ -336,7 +341,7 @@ static void spapr_xive_realize(DeviceState *dev, Error 
> **errp)
>   * Allocate the routing tables
>   */
>  xive->eat = g_new0(XiveEAS, xive->nr_irqs);
> -xive->endt = g_new0(XiveEND, xive->nr_ends);
> +xive->endt = g_new0(XiveEND, spapr_xive_nr_ends(xive));
>  
>  xive->nodename = g_strdup_printf("interrupt-controller@%" PRIx64,
> xive->tm_base + XIVE_TM_USER_PAGE * (1 << 
> TM_SHIFT));
> @@ -375,7 +380,7 @@ static int spapr_xive_get_end(XiveRouter *xrtr,
>  {
>  SpaprXive *xive = SPAPR_XIVE(xrtr);
>  
> -if (end_idx >= xive->nr_ends) {
> +if (end_idx >= spapr_xive_nr_ends(xive)) {
>  return -1;
>  }
>  
> @@ -389,7 +394,7 @@ static int spapr_xive_write_end(XiveRouter *xrtr, uint8_t 
> end_blk,
>  {
>  SpaprXive *xive = SPAPR_XIVE(xrtr);
>  
> -if (end_idx >= xive->nr_ends) {
> +if (end_idx >= spapr_xive_nr_ends(xive)) {
>  return -1;
>  }
>  
> @@ -1138,7 +1143,7 @@ static target_ulong h_int_get_source_config(PowerPCCPU 
> *cpu,
>  /* EAS_END_BLOCK is unused on sPAPR */
>  end_idx = xive_get_field64(EAS_END_INDEX, eas.w);
>  
> -assert(end_idx < xive->nr_ends);
> +assert(end_idx < spapr_xive_nr_ends(xive));
>  end = >endt[end_idx];
>  
>  nvt_blk = xive_get_field32(END_W6_NVT_BLOCK, end->w6);
> @@ -1216,7 +1221,7 @@ static target_ulong h_int_get_queue_info(PowerPCCPU 
> *cpu,
>  return H_P2;
>  }
>  
> -assert(end_idx < xive->nr_ends);
> +assert(end_idx < spapr_xive_nr_ends(xive));
>  end = >endt[end_idx];
>  
>  args[0] = xive->end_base + (1ull << (end_xsrc->esb_shift + 1)) * end_idx;
> @@ -1304,7 +1309,7 @@ static target_ulong h_int_set_queue_config(PowerPCCPU 
> *cpu,
>  return H_P2;
>  }
>  
> -assert(end_idx < xive->nr_ends);
> +assert(end_idx < spapr_xive_nr_ends(xive));
>  memcpy(, >endt[end_idx], sizeof(XiveEND));
>  
>  switch (qsize) {
> @@ -1470,7 +1475,7 @@ static target_ulong h_int_get_queue_config(PowerPCCPU 
> *cpu,
>  return H_P2;
>  }
>  
> -assert(end_idx < xive->nr_ends);
> +assert(end_idx < spapr_xive_nr_ends(xive));
>  end = >endt[end_idx];
>  
>  args[0] = 0;
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index 66bf4c06fe55..1566016f0e28 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -531,7 +531,7 @@ static int kvmppc_xive_get_queues(SpaprXive *xive, Error 
> **errp)
>  int i;
>  int ret;
>  
> -for (i = 0; i < xive->nr_ends; i++) {
> +for (i = 0; i < spapr_xive_nr_ends(xive); i++) {
>  if (!xive_end_is_valid(>endt[i])) {
>  continue;
>  }
> @@ -701,7 +701,7 @@ int kvmppc_xive_post_load(SpaprXive *xive, int version_id)
>  assert(xive->fd != -1);
>  
>  /* Restore the ENDT first. The 

Re: [PATCH for-6.0 3/8] spapr/xive: Add "nr-servers" property

[David Gibson]

On Fri, Nov 20, 2020 at 06:46:41PM +0100, Greg Kurz wrote:
> The sPAPR XIVE object has an "nr-ends" property that is used
> to size the END table. This property is set by the machine
> code to a value derived from spapr_max_server_number().
> 
> spapr_max_server_number() is also used to inform the KVM XIVE
> device about the range of vCPU ids it might be exposed to,
> in order to optimize resource allocation in the HW.
> 
> This is enough motivation to introduce an "nr-servers" property
> and to use it for both purposes. The existing "nr-ends" property
> is now longer used. It is kept around though because it is exposed
> to -global. It will continue to be ignored as before without
> causing QEMU to exit.

I'm a little dubious about keeping the property around.  It's
technically a breaking change to remove it, but since IIUC, it's
*never* had any effect, it seems implausible anyone out there's using
it.

Can we at least put it straight into the deprecation document?

> The associated nr_ends field cannot be dropped from SpaprXive
> because it is explicitly used by vmstate_spapr_xive(). It is
> thus renamed to nr_ends_vmstate.
> 
> Signed-off-by: Greg Kurz 
> ---
>  include/hw/ppc/spapr_xive.h | 16 +++-
>  hw/intc/spapr_xive.c| 28 ++--
>  hw/ppc/spapr_irq.c  |  6 +-
>  3 files changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index 4b967f13c030..7123ea07ed78 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -23,6 +23,16 @@
>  typedef struct SpaprXive {
>  XiveRouterparent;
>  
> +/*
> + * The XIVE device needs to know the highest vCPU id it might
> + * be exposed to in order to size the END table. It may also
> + * propagate the value to the KVM XIVE device in order to
> + * optimize resource allocation in the HW.
> + * This must be set to a non-null value using the "nr-servers"
> + * property, before realizing the device.
> + */
> +uint32_t  nr_servers;
> +
>  /* Internal interrupt source for IPIs and virtual devices */
>  XiveSourcesource;
>  hwaddrvc_base;
> @@ -38,7 +48,11 @@ typedef struct SpaprXive {
>  XiveEAS   *eat;
>  uint32_t  nr_irqs;
>  XiveEND   *endt;
> -uint32_t  nr_ends;
> +/*
> + * This is derived from nr_servers but it must be kept around because
> + * vmstate_spapr_xive uses it.
> + */
> +uint32_t  nr_ends_vmstate;
>  
>  /* TIMA mapping address */
>  hwaddrtm_base;
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index f473ad9cba47..e4dbf9c2c409 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -99,6 +99,12 @@ int spapr_xive_end_to_target(uint8_t end_blk, uint32_t 
> end_idx,
>  return 0;
>  }
>  
> +/*
> + * 8 XIVE END structures per CPU. One for each available
> + * priority
> + */
> +#define spapr_xive_cpu_end_idx(vcpu, prio) (((vcpu) << 3) + prio)
> +
>  static void spapr_xive_cpu_to_end(PowerPCCPU *cpu, uint8_t prio,
>uint8_t *out_end_blk, uint32_t 
> *out_end_idx)
>  {
> @@ -109,7 +115,7 @@ static void spapr_xive_cpu_to_end(PowerPCCPU *cpu, 
> uint8_t prio,
>  }
>  
>  if (out_end_idx) {
> -*out_end_idx = (cpu->vcpu_id << 3) + prio;
> +*out_end_idx = spapr_xive_cpu_end_idx(cpu->vcpu_id, prio);
>  }
>  }
>  
> @@ -290,7 +296,8 @@ static void spapr_xive_instance_init(Object *obj)
>  
>  uint32_t spapr_xive_nr_ends(const SpaprXive *xive)
>  {
> -return xive->nr_ends;
> +g_assert(xive->nr_servers);
> +return spapr_xive_cpu_end_idx(xive->nr_servers, 0);
>  }
>  
>  static void spapr_xive_realize(DeviceState *dev, Error **errp)
> @@ -303,7 +310,7 @@ static void spapr_xive_realize(DeviceState *dev, Error 
> **errp)
>  
>  /* Set by spapr_irq_init() */
>  g_assert(xive->nr_irqs);
> -g_assert(xive->nr_ends);
> +g_assert(xive->nr_servers);
>  
>  sxc->parent_realize(dev, _err);
>  if (local_err) {
> @@ -360,6 +367,8 @@ static void spapr_xive_realize(DeviceState *dev, Error 
> **errp)
>  sysbus_mmio_map(SYS_BUS_DEVICE(xive), 0, xive->vc_base);
>  sysbus_mmio_map(SYS_BUS_DEVICE(xive), 1, xive->end_base);
>  sysbus_mmio_map(SYS_BUS_DEVICE(xive), 2, xive->tm_base);
> +
> +xive->nr_ends_vmstate = spapr_xive_nr_ends(xive);
>  }
>  
>  static int spapr_xive_get_eas(XiveRouter *xrtr, uint8_t eas_blk,
> @@ -547,7 +556,7 @@ static const VMStateDescription vmstate_spapr_xive = {
>  VMSTATE_UINT32_EQUAL(nr_irqs, SpaprXive, NULL),
>  VMSTATE_STRUCT_VARRAY_POINTER_UINT32(eat, SpaprXive, nr_irqs,
>   vmstate_spapr_xive_eas, XiveEAS),
> -VMSTATE_STRUCT_VARRAY_POINTER_UINT32(endt, SpaprXive, nr_ends,
> +VMSTATE_STRUCT_VARRAY_POINTER_UINT32(endt, SpaprXive, 
> nr_ends_vmstate,
>   

Re: [PATCH for-6.0 4/8] spapr/xive: Add "nr-ipis" property

[David Gibson]

On Fri, Nov 20, 2020 at 06:46:42PM +0100, Greg Kurz wrote:
> The sPAPR XIVE device exposes a range of LISNs that the guest uses
> for IPIs. This range is currently sized according to the highest
> vCPU id, ie. spapr_max_server_number(), as obtained from the machine
> through the "nr_servers" argument of the generic spapr_irq_dt() call.
> 
> This makes sense for the "ibm,interrupt-server-ranges" property of
> sPAPR XICS, but certainly not for "ibm,xive-lisn-ranges". The range
> should be sized to the maximum number of possible vCPUs. It happens
> that spapr_max_server_number() == smp.max_cpus in practice with the
> machine default settings. This might not be the case though when
> VSMT is in use : we can end up with a much larger range (up to 8
> times bigger) than needed and waste LISNs. But most importantly, this
> lures people into thinking that IPI numbers are always equal to
> vCPU ids, which is wrong and bit us recently:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg01476.html
> 
> Introduce an "nr-ipis" property that the machine sets to smp.max_cpus
> before realizing the deice. Use that instead of "nr_servers" in
> spapr_xive_dt(). Have the machine to claim the same number of IPIs
> in spapr_irq_init().
> 
> This doesn't cause any guest visible change when using the machine
> default settings (ie. VSMT == smp.threads).
> 
> Signed-off-by: Greg Kurz 

Reviewed-by: David Gibson 

> ---
>  include/hw/ppc/spapr_xive.h | 8 
>  hw/intc/spapr_xive.c| 4 +++-
>  hw/ppc/spapr_irq.c  | 4 +++-
>  3 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index 7123ea07ed78..69b9fbc1b9a5 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -43,6 +43,14 @@ typedef struct SpaprXive {
>  
>  /* DT */
>  gchar *nodename;
> +/*
> + * The sPAPR XIVE device needs to know how many vCPUs it
> + * might be exposed to in order to size the IPI range in
> + * "ibm,interrupt-server-ranges". It is the purpose of the
> + * "nr-ipis" property which *must* be set to a non-null
> + * value before realizing the sPAPR XIVE device.
> + */
> +uint32_t nr_ipis;
>  
>  /* Routing table */
>  XiveEAS   *eat;
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index e4dbf9c2c409..d13a2955ce9b 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -311,6 +311,7 @@ static void spapr_xive_realize(DeviceState *dev, Error 
> **errp)
>  /* Set by spapr_irq_init() */
>  g_assert(xive->nr_irqs);
>  g_assert(xive->nr_servers);
> +g_assert(xive->nr_ipis);
>  
>  sxc->parent_realize(dev, _err);
>  if (local_err) {
> @@ -608,6 +609,7 @@ static Property spapr_xive_properties[] = {
>   */
>  DEFINE_PROP_UINT32("nr-ends", SpaprXive, nr_ends_vmstate, 0),
>  DEFINE_PROP_UINT32("nr-servers", SpaprXive, nr_servers, 0),
> +DEFINE_PROP_UINT32("nr-ipis", SpaprXive, nr_ipis, 0),
>  DEFINE_PROP_UINT64("vc-base", SpaprXive, vc_base, SPAPR_XIVE_VC_BASE),
>  DEFINE_PROP_UINT64("tm-base", SpaprXive, tm_base, SPAPR_XIVE_TM_BASE),
>  DEFINE_PROP_UINT8("hv-prio", SpaprXive, hv_prio, 7),
> @@ -698,7 +700,7 @@ static void spapr_xive_dt(SpaprInterruptController *intc, 
> uint32_t nr_servers,
>  /* Interrupt number ranges for the IPIs */
>  uint32_t lisn_ranges[] = {
>  cpu_to_be32(SPAPR_IRQ_IPI),
> -cpu_to_be32(SPAPR_IRQ_IPI + nr_servers),
> +cpu_to_be32(SPAPR_IRQ_IPI + xive->nr_ipis),
>  };
>  /*
>   * EQ size - the sizes of pages supported by the system 4K, 64K,
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 8c5627225636..a0fc474ecb06 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -325,12 +325,14 @@ void spapr_irq_init(SpaprMachineState *spapr, Error 
> **errp)
>  
>  if (spapr->irq->xive) {
>  uint32_t nr_servers = spapr_max_server_number(spapr);
> +uint32_t max_cpus = MACHINE(spapr)->smp.max_cpus;
>  DeviceState *dev;
>  int i;
>  
>  dev = qdev_new(TYPE_SPAPR_XIVE);
>  qdev_prop_set_uint32(dev, "nr-irqs", smc->nr_xirqs + 
> SPAPR_XIRQ_BASE);
>  qdev_prop_set_uint32(dev, "nr-servers", nr_servers);
> +qdev_prop_set_uint32(dev, "nr-ipis", max_cpus);
>  object_property_set_link(OBJECT(dev), "xive-fabric", OBJECT(spapr),
>   _abort);
>  sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
> @@ -338,7 +340,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error 
> **errp)
>  spapr->xive = SPAPR_XIVE(dev);
>  
>  /* Enable the CPU IPIs */
> -for (i = 0; i < nr_servers; ++i) {
> +for (i = 0; i < max_cpus; ++i) {
>  SpaprInterruptControllerClass *sicc
>  = SPAPR_INTC_GET_CLASS(spapr->xive);
>  

-- 
David Gibson| I'll have my music 

[Bug 1905226] [NEW] intel-hda: stream reset bits are broken

[Jacob]

Public bug reported:

>From HD audio spec, section 3.3.35:

"Stream Reset (SRST): Writing a 1 causes the corresponding stream to be
reset. [...] After the stream hardware has completed sequencing into the
reset state, it will report a 1 in this bit. Software must read a 1 from
this bit to verify that the stream is in reset. Writing a 0 causes the
corresponding stream to exit reset. When the stream hardware is ready to
begin operation, it will report a 0 in this bit. Software must read a 0
from this bit before accessing any of the stream registers."

So to reset a stream I set the bit, but it never reads back as 1 so the
driver either times out or will hang forever waiting for it to become 1.
I looked into why this happens and found that as of the latest version
(8110fa1), in function intel_hda_set_st_ctl() of the
https://github.com/qemu/qemu/blob/master/hw/audio/intel-hda.c,

if (st->ctl & 0x01) {
/* reset */
dprint(d, 1, "st #%d: reset\n", reg->stream);
st->ctl = SD_STS_FIFO_READY << 24;
}

This causes the bit to immediately become set to 0 even if I write a 1,
and clearly does not meet the spec. I checked behaviour of real hardware
and it works as expected, i.e. I see the bit will become 1 and 0 when I
write to it.

** Affects: qemu
 Importance: Undecided
 Status: New


** Tags: intel-hda

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1905226

Title:
  intel-hda: stream reset bits are broken

Status in QEMU:
  New

Bug description:
  From HD audio spec, section 3.3.35:

  "Stream Reset (SRST): Writing a 1 causes the corresponding stream to
  be reset. [...] After the stream hardware has completed sequencing
  into the reset state, it will report a 1 in this bit. Software must
  read a 1 from this bit to verify that the stream is in reset. Writing
  a 0 causes the corresponding stream to exit reset. When the stream
  hardware is ready to begin operation, it will report a 0 in this bit.
  Software must read a 0 from this bit before accessing any of the
  stream registers."

  So to reset a stream I set the bit, but it never reads back as 1 so
  the driver either times out or will hang forever waiting for it to
  become 1. I looked into why this happens and found that as of the
  latest version (8110fa1), in function intel_hda_set_st_ctl() of the
  https://github.com/qemu/qemu/blob/master/hw/audio/intel-hda.c,

  if (st->ctl & 0x01) {
  /* reset */
  dprint(d, 1, "st #%d: reset\n", reg->stream);
  st->ctl = SD_STS_FIFO_READY << 24;
  }

  This causes the bit to immediately become set to 0 even if I write a
  1, and clearly does not meet the spec. I checked behaviour of real
  hardware and it works as expected, i.e. I see the bit will become 1
  and 0 when I write to it.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1905226/+subscriptions

Re: [PATCH RFC] vfio: Move the saving of the config space to the right place in VFIO migration

[Shenming Lu]

On 2020/11/21 6:01, Alex Williamson wrote:
> On Fri, 20 Nov 2020 22:05:49 +0800
> Shenming Lu  wrote:
> 
>> On 2020/11/20 1:41, Alex Williamson wrote:
>>> On Thu, 19 Nov 2020 14:13:24 +0530
>>> Kirti Wankhede  wrote:
>>>   
 On 11/14/2020 2:47 PM, Shenming Lu wrote:  
> When running VFIO migration, I found that the restoring of VFIO PCI 
> device’s
> config space is before VGIC on ARM64 target. But generally, interrupt 
> controllers
> need to be restored before PCI devices. 

 Is there any other way by which VGIC can be restored before PCI device?  
>>
>> As far as I know, it seems to have to depend on priorities in the 
>> non-iterable process.
>>
  
> Besides, if a VFIO PCI device is
> configured to have directly-injected MSIs (VLPIs), the restoring of its 
> config
> space will trigger the configuring of these VLPIs (in kernel), where it 
> would
> return an error as I saw due to the dependency on kvm’s vgic.
> 

 Can this be fixed in kernel to re-initialize the kernel state?  
>>
>> Did you mean to reconfigure these VLPIs when restoring kvm's vgic?
>> But the fact is that this error is not caused by kernel, it is due to the 
>> incorrect
>> calling order of qemu...
>>
  
> To avoid this, we can move the saving of the config space from the 
> iterable
> process to the non-iterable process, so that it will be called after VGIC
> according to their priorities.
> 

 With this change, at resume side, pre-copy phase data would reach 
 destination without restored config space. VFIO device on destination 
 might need it's config space setup and validated before it can accept 
 further VFIO device specific migration state.

 This also changes bit-stream, so it would break migration with original 
 migration patch-set.  
>>>
>>> Config space can continue to change while in pre-copy, if we're only
>>> sending config space at the initiation of pre-copy, how are any changes
>>> that might occur before the VM is stopped conveyed to the target?  For
>>> example the guest might reboot and a device returned to INTx mode from
>>> MSI during pre-copy.  Thanks,  
>>
>> What I see is that the config space is only saved once in 
>> save_live_complete_precopy
>> currently...
>> As you said, a VFIO device might need it's config space setup first, and
>> the config space can continue to change while in pre-copy, Did you mean we
>> have to migrate the config space in save_live_iterate?
>> However, I still have a little doubt about the restoring dependence between
>> the qemu emulated config space and the device data...
>>
>> Besides, if we surely can't move the saving of the config space back, can we
>> just move some actions which are triggered by the restoring of the config 
>> space
>> back (such as vfio_msix_enable())?
> 
> It seems that the significant benefit to enabling interrupts during
> pre-copy would be to reduce the latency and failure potential during
> the final phase of migration.  Do we have any data for how much it adds
> to the device contributed downtime to configure interrupts only at the
> final stage?  My guess is that it's a measurable delay on its own.  At
> the same time, we can't ignore the differences in machine specific
> dependencies and if we don't even sync the config space once the VM is
> stopped... this all seems not ready to call supported, especially if we
> have concerns already about migration bit-stream compatibility.
> 

I have another question for this, if we restore the config space while in 
pre-copy
(include enabling interrupts), does it affect the _RESUMING state (paused) of 
the
device on the dst host (cause it to send interrupts? which should not be allowed
in this stage). Does the restore sequence need to be further discussed and reach
a consensus(spec) (taking into account other devices and the corresponding 
actions
of the vendor driver)?

> Given our timing relative to QEMU 5.2, the only path I feel comfortable
> with is to move forward with downgrading vfio migration support to be
> enabled via an experimental option.  Objections?  Thanks,

Alright, but this issue is related to our ARM GICv4.1 migration scheme, could 
you
give a rough idea about this (where to enable interrupts, we hope it to be after
the restoring of VGIC)?

Thanks,
Shenming

Re: [PATCH 1/1] /net/tap.c: Fix a memory leak

[Jason Wang]

On 2020/11/22 下午7:39, Peter Maydell wrote:

On Sun, 22 Nov 2020 at 11:07,  wrote:

From: yuanjungong 

Close fd before returning.

Buglink: https://bugs.launchpad.net/qemu/+bug/1904486
Signed-off-by: yuanjungong 
---
  net/tap.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/net/tap.c b/net/tap.c
index c46ff66..fe95fa7 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -817,6 +817,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
  if (ret < 0) {
  error_setg_errno(errp, -ret, "%s: Can't use file descriptor %d",
   name, fd);
+close(fd);
  return -1;
  }

@@ -831,6 +832,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
   vhostfdname, vnet_hdr, fd, );
  if (err) {
  error_propagate(errp, err);
+close(fd);
  return -1;
  }
  } else if (tap->has_fds) {
--
2.17.1

Reviewed-by: Peter Maydell 

thanks
-- PMM



Applied.

Thanks

Re: [PATCH] ui/vnc: Add missing lock for send_color_map

[Peng Liang]

Kindly ping.

On 11/16/2020 10:13 PM, Peng Liang wrote:
> vnc_write() should be locked after the RFB protocol is initialized.
> 
> Fixes: 0c426e4534b4 ("vnc: Add support for color map")
> Cc: qemu-sta...@nongnu.org
> Reported-by: Euler Robot 
> Signed-off-by: Peng Liang 
> ---
>  ui/vnc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 49235056f7a8..ca3fc376aeb5 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -2156,6 +2156,7 @@ static void send_color_map(VncState *vs)
>  {
>  int i;
>  
> +vnc_lock_output(vs);
>  vnc_write_u8(vs, VNC_MSG_SERVER_SET_COLOUR_MAP_ENTRIES);
>  vnc_write_u8(vs,  0);/* padding */
>  vnc_write_u16(vs, 0);/* first color */
> @@ -2168,6 +2169,7 @@ static void send_color_map(VncState *vs)
>  vnc_write_u16(vs, (((i >> pf->gshift) & pf->gmax) << (16 - 
> pf->gbits)));
>  vnc_write_u16(vs, (((i >> pf->bshift) & pf->bmax) << (16 - 
> pf->bbits)));
>  }
> +vnc_unlock_output(vs);
>  }
>  
>  static void set_pixel_format(VncState *vs, int bits_per_pixel,
> 

Thanks,
Peng

Re: [RFC 15/15] target/riscv: rvb: support and turn on B-extension from command line

[Frank Chang]

On Sat, Nov 21, 2020 at 12:24 AM Alistair Francis 
wrote:

>
>
> On 19/11/2020 7:02 pm, Kito Cheng wrote:
> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >> index 0bbfd7f4574..bc29e118c6d 100644
> >> --- a/target/riscv/cpu.c
> >> +++ b/target/riscv/cpu.c
> >> @@ -438,6 +438,9 @@ static void riscv_cpu_realize(DeviceState *dev,
> Error **errp)
> >>   if (cpu->cfg.ext_h) {
> >>   target_misa |= RVH;
> >>   }
> >> +if (cpu->cfg.ext_b) {
> >> +target_misa |= RVB;
> >> +}
> >>   if (cpu->cfg.ext_v) {
> >>   target_misa |= RVV;
> >>   if (!is_power_of_2(cpu->cfg.vlen)) {
> >> @@ -515,6 +518,7 @@ static Property riscv_cpu_properties[] = {
> >>   DEFINE_PROP_BOOL("s", RISCVCPU, cfg.ext_s, true),
> >>   DEFINE_PROP_BOOL("u", RISCVCPU, cfg.ext_u, true),
> >>   /* This is experimental so mark with 'x-' */
> >> +DEFINE_PROP_BOOL("x-b", RISCVCPU, cfg.ext_b, true),
> >
> > I think the default value should be false?
>
> Good catch, I missed that.
>
> Yes it should be false.
>
> Alistair
>

Thanks, I'll fix it in my next patchset.

Frank Chang


>
> >
> >>   DEFINE_PROP_BOOL("x-h", RISCVCPU, cfg.ext_h, false),
> >>   DEFINE_PROP_BOOL("x-v", RISCVCPU, cfg.ext_v, false),
> >>   DEFINE_PROP_BOOL("Counters", RISCVCPU, cfg.ext_counters, true),
> >
>

Re: [PATCH 23/26] target/mips: Extract Toshiba TX79 multimedia translation routines

[Philippe Mathieu-Daudé]

On 11/21/20 9:17 PM, Richard Henderson wrote:
> On 11/20/20 1:08 PM, Philippe Mathieu-Daudé wrote:
>> +++ b/target/mips/vendor-tx_translate.c.inc
>> @@ -41,6 +41,8 @@
>>  
>>  #if defined(TARGET_MIPS64)
>>  
>> +#include "vendor-tx-mmi_translate.c.inc"
> 
> Do you really want to nest include files like this?

Can do better, indeed ;)

> 
> 
> r~
> 

Re: [PATCH v2] target/mips/helper: Also display exception names in user-mode

[Philippe Mathieu-Daudé]

On 11/19/20 5:05 PM, Philippe Mathieu-Daudé wrote:
> Currently MIPS exceptions are displayed as string in system-mode
> emulation, but as number in user-mode.
> Unify by extracting the current system-mode code as excp_name()
> and use that in user-mode.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> Since v1: Fixed failed cherry-pick conflict resolution
> ---
>  target/mips/helper.c | 24 +---
>  1 file changed, 13 insertions(+), 11 deletions(-)

Thanks, applied to mips-next.

Re: [PATCH-for-6.0 0/2] target/mips: CP0 housekeeping patches for Nov 2020

[Philippe Mathieu-Daudé]

On 11/9/20 10:04 AM, Philippe Mathieu-Daudé wrote:
> Based-on: <20201108234234.2389789-1-f4...@amsat.org>
> 
> Philippe Mathieu-Daudé (2):
>   target/mips: Replace magic values by CP0PM_MASK or
> TARGET_PAGE_BITS_MIN
>   target/mips: Do not include CP0 helpers in user-mode emulation
> 
>  target/mips/cp0_helper.c | 9 +++--
>  target/mips/helper.c | 4 ++--
>  target/mips/meson.build  | 2 +-
>  3 files changed, 6 insertions(+), 9 deletions(-)

Thanks, applied to mips-next.

Re: [PULL 0/1] qemu-sparc queue 20201122

[Peter Maydell]

On Sun, 22 Nov 2020 at 14:27, Mark Cave-Ayland
 wrote:
>
> The following changes since commit e3a232cccd2445e5d9e607a65a78cdbc33ff8a0f:
>
>   Merge remote-tracking branch 'remotes/bonzini-gitlab/tags/for-upstream' 
> into staging (2020-11-20 22:30:51 +)
>
> are available in the Git repository at:
>
>   git://github.com/mcayland/qemu.git tags/qemu-sparc-20201122
>
> for you to fetch changes up to 48e5c7f34c557afe49396a00169629d24dc3342d:
>
>   hw/display/tcx: add missing 64-bit access for framebuffer blitter 
> (2020-11-22 10:43:30 +)
>
> 
> qemu-sparc queue
>
> 
> Mark Cave-Ayland (1):
>   hw/display/tcx: add missing 64-bit access for framebuffer blitter


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.2
for any user-visible changes.

-- PMM

Re: [PATCH 20/26] target/mips: Extract XBurst Media eXtension Unit translation routines

[Philippe Mathieu-Daudé]

On 11/21/20 9:13 PM, Richard Henderson wrote:
> On 11/20/20 1:08 PM, Philippe Mathieu-Daudé wrote:
>> Media eXtension Unit is a SIMD extension from Ingenic.
>>
>> Extract 2900 lines from the huge translate.c to a new file,
>> 'vendor-xburst_translate.c.inc'. As there are too many inter-
>> dependencies we don't compile it as another object, but
>> keep including it in the big translate.o. We gain in code
>> maintainability.
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  target/mips/translate.c   | 2890 +---
>>  target/mips/vendor-xburst_translate.c.inc | 2893 +
>>  2 files changed, 2894 insertions(+), 2889 deletions(-)
>>  create mode 100644 target/mips/vendor-xburst_translate.c.inc
> 
> Where does the xburst name come from?
> It's a little confusing that all of the comments talk about MXU but the
> filename is xburst.  Perhaps add a comment near the top identifying the 
> rename,
> if you can?

This is in the first comment:

 *   "XBurst® Instruction Set Architecture MIPS eXtension/enhanced Unit
 *   Programming Manual", Ingenic Semiconductor Co, Ltd., revision June
2, 2017

But it appears at the end, and the comment is 340+ lines...

I'll move it to the top.

Phil.
> 
> 
> r~
> 

Re: [PATCH 19/26] target/mips: Extract Loongson EXTensions translation routines

[Philippe Mathieu-Daudé]

On 11/21/20 9:10 PM, Richard Henderson wrote:
> On 11/20/20 1:08 PM, Philippe Mathieu-Daudé wrote:
>> LoongEXTs are general-purpose extensions from the LoongISA.
>>
>> Extract 440 lines of translation routines to
>> 'vendor-loong-lext_translate.c.inc'.
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  target/mips/translate.c   | 440 +
>>  target/mips/vendor-loong-lext_translate.c.inc | 450 ++
>>  2 files changed, 451 insertions(+), 439 deletions(-)
>>  create mode 100644 target/mips/vendor-loong-lext_translate.c.inc
> 
> s/lext/ext/?  Seems silly to have loong in there twice.

OK (this is listed as LoongEXT32/LoongEXT64 in the datasheets).

> And I presume this is where gen_loong_integer goes?

Yes.

> 
> 
> r~
> 

Re: [PATCH 02/26] target/mips: Extract MSA helpers to mod-mips-msa_helper.c

[Philippe Mathieu-Daudé]

On 11/21/20 8:44 PM, Richard Henderson wrote:
> On 11/20/20 1:08 PM, Philippe Mathieu-Daudé wrote:
>> MSA means 'MIPS SIMD Architecture' and is defined as a Module by MIPS.
>> Rename msa_helper.c as mod-mips-msa_helper.c, merge other MSA helpers
>> from op_helper.c.
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  .../{msa_helper.c => mod-mips-msa_helper.c}   | 392 +
>>  target/mips/op_helper.c   | 393 --
>>  target/mips/meson.build   |   3 +-
>>  3 files changed, 394 insertions(+), 394 deletions(-)
>>  rename target/mips/{msa_helper.c => mod-mips-msa_helper.c} (94%)
> 
> Do we really need "mips" in the filename, given that it's implied by the 
> directory?

Indeed we don't need it :)

> 
> Maybe perform the rename and the op_helper.c move in different patches?

Sure, will do.

Regards,

Phil.

Re: [PATCH 01/26] target/mips: Extract FPU helpers to 'fpu_helper.h'

[Philippe Mathieu-Daudé]

On 11/21/20 8:39 PM, Richard Henderson wrote:
> On 11/20/20 1:08 PM, Philippe Mathieu-Daudé wrote:
>> Extract FPU specific helpers from "internal.h" to "fpu_helper.h".
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  target/mips/fpu_helper.h   | 50 ++
>>  target/mips/internal.h | 42 
>>  linux-user/mips/cpu_loop.c |  1 +
>>  target/mips/fpu_helper.c   |  1 +
>>  target/mips/gdbstub.c  |  1 +
>>  target/mips/kvm.c  |  1 +
>>  target/mips/machine.c  |  1 +
>>  target/mips/msa_helper.c   |  1 +
>>  target/mips/translate.c|  1 +
>>  9 files changed, 57 insertions(+), 42 deletions(-)
>>  create mode 100644 target/mips/fpu_helper.h
> 
> Reviewed-by: Richard Henderson 
> 
>> +extern unsigned int ieee_rm[];
> 
> Note for future cleanup: const FloatRoundMode.

Good point, cleaner.

Thanks!

Phil.

Re: [Bug 645662] Re: QEMU x87 emulation of trig and other complex ops is only at 64-bit precision, not 80-bit

[Arno Wagner]

Talk about a "blast form the past!" This Bug is now over 10 years old. 
But at least somebody is still working on it and it was not
just quietly dropped. I can respect that. 

My original recommendation stands: At least use long double for the
calcuations where available. 

Regards,
Arno

On Sun, Nov 22, 2020 at 00:21:50 CET, Peter Maydell wrote:
> For 5.1 (commits 1f18a1e6ab8368a4, 5eebc49d2d0aa5fc7e,
> 5ef396e2ba865f34a, eca30647fc078f4) we reimplemented FPATAN, FYL2X,
> FYL2XP1, FPREM, FPREM1, F2XM1 to do proper 80-bit precision operations.
> However the trig operations FPTAN, FSINCOS, FSIN, FCOS are still
> implemented as naive "convert to host double and use host C library
> functions".
> 
> -- 
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/645662
> 
> Title:
>   QEMU x87 emulation of trig and other complex ops is only at 64-bit
>   precision, not 80-bit
> 
> Status in QEMU:
>   Confirmed
> 
> Bug description:
>   When doing the regression tests for Python 3.1.2 with Qemu 0.12.5, (Linux 
> version 2.6.26-2-686 (Debian 2.6.26-25lenny1)),
>   gcc (Debian 4.3.2-1.1) 4.3.2, Python compiled from sources within qemu,
>   3 math tests fail, apparently because the floating point unit is buggy. 
> Qmeu was compiled from original sources
>   on Debian Lenny with kernel  2.6.34.6 from kernel.org, gcc  (Debian 
> 4.3.2-1.1) 4.3. 
> 
>   Regression testing errors:
> 
>   test_cmath
>   test test_cmath failed -- Traceback (most recent call last):
> File "/root/tools/python3/Python-3.1.2/Lib/test/test_cmath.py", line 364, 
> in
>   self.fail(error_message)
>   AssertionError: acos0034: acos(complex(-1.0002, 0.0))
>   Expected: complex(3.141592653589793, -2.1073424255447014e-08)
>   Received: complex(3.141592653589793, -2.1073424338879928e-08)
>   Received value insufficiently close to expected value.
> 
>   
>   test_float
>   test test_float failed -- Traceback (most recent call last):
> File "/root/tools/python3/Python-3.1.2/Lib/test/test_float.py", line 479, 
> in
>   self.assertEqual(s, repr(float(s)))
>   AssertionError: '8.72293771110361e+25' != '8.722937711103609e+25'
> 
>   
>   test_math
>   test test_math failed -- multiple errors occurred; run in verbose mode for 
> deta
> 
>   =>
> 
>   runtests.sh -v test_math
> 
>   le01:~/tools/python3/Python-3.1.2# ./runtests.sh -v test_math
>   test_math BAD
>1 BAD
>0 GOOD
>0 SKIPPED
>1 total
>   le01:~/tools/python3/Python-3.1.2#
> 
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/qemu/+bug/645662/+subscriptions

-- 
Arno Wagner, Dr. sc. techn., Dipl. Inform.,Email: a...@wagner.name
GnuPG: ID: CB5D9718  FP: 12D6 C03B 1B30 33BB 13CF  B774 E35C 5FA1 CB5D 9718

A good decision is based on knowledge and not on numbers. -- Plato

If it's in the news, don't worry about it.  The very definition of 
"news" is "something that hardly ever happens." -- Bruce Schneier

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/645662

Title:
  QEMU x87 emulation of trig and other complex ops is only at 64-bit
  precision, not 80-bit

Status in QEMU:
  Confirmed

Bug description:
  When doing the regression tests for Python 3.1.2 with Qemu 0.12.5, (Linux 
version 2.6.26-2-686 (Debian 2.6.26-25lenny1)),
  gcc (Debian 4.3.2-1.1) 4.3.2, Python compiled from sources within qemu,
  3 math tests fail, apparently because the floating point unit is buggy. Qmeu 
was compiled from original sources
  on Debian Lenny with kernel  2.6.34.6 from kernel.org, gcc  (Debian 
4.3.2-1.1) 4.3. 

  Regression testing errors:

  test_cmath
  test test_cmath failed -- Traceback (most recent call last):
File "/root/tools/python3/Python-3.1.2/Lib/test/test_cmath.py", line 364, in
  self.fail(error_message)
  AssertionError: acos0034: acos(complex(-1.0002, 0.0))
  Expected: complex(3.141592653589793, -2.1073424255447014e-08)
  Received: complex(3.141592653589793, -2.1073424338879928e-08)
  Received value insufficiently close to expected value.

  
  test_float
  test test_float failed -- Traceback (most recent call last):
File "/root/tools/python3/Python-3.1.2/Lib/test/test_float.py", line 479, in
  self.assertEqual(s, repr(float(s)))
  AssertionError: '8.72293771110361e+25' != '8.722937711103609e+25'

  
  test_math
  test test_math failed -- multiple errors occurred; run in verbose mode for 
deta

  =>

  runtests.sh -v test_math

  le01:~/tools/python3/Python-3.1.2# ./runtests.sh -v test_math
  test_math BAD
   1 BAD
   0 GOOD
   0 SKIPPED
   1 total
  le01:~/tools/python3/Python-3.1.2#

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/645662/+subscriptions

Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[Yuri Benditovich]

I'm sorry for the confusion. Andrew's description of steps to reproduce the
problem is correct.
I've described another problem present in the master but not related to
this patch.

On Sun, Nov 22, 2020 at 11:45 AM Andrew Melnichenko 
wrote:

> Hi, the bug can be reproduced like that:
>
>> QEMU 5.1.50 monitor - type 'help' for more information
>> (qemu) netdev_add
>> type=tap,id=net0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no
>> (qemu) info network
>> hub 0
>>  \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off
>>  \ hub0port0: e1000e.0:
>> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56
>> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
>> net0:
>> index=0,type=tap,ifname=tap0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no
>> (qemu) netdev_del net0
>> (qemu) info network
>> hub 0
>>  \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off
>>  \ hub0port0: e1000e.0:
>> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56
>> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
>> (qemu) netdev_add
>> type=tap,id=net0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no
>> Try "help netdev_add" for more information
>> (qemu) info network
>> hub 0
>>  \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off
>>  \ hub0port0: e1000e.0:
>> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56
>> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
>> (qemu)
>>
>>
> Its still actual bug - I've checked it with the
> master(2c6605389c1f76973d92b69b85d40d94b8f1092c).
>
> On Sat, Nov 21, 2020 at 5:31 PM Yuri Benditovich <
> yuri.benditov...@daynix.com> wrote:
>
>>
>>
>> On Sat, Nov 21, 2020 at 5:24 PM Yuri Benditovich <
>> yuri.benditov...@daynix.com> wrote:
>>
>>>
>>>
>>> On Fri, Nov 20, 2020 at 2:58 PM Markus Armbruster 
>>> wrote:
>>>
 Andrew Melnichenko  writes:

 > Ping
 >
 > On Thu, Jul 16, 2020 at 6:26 AM  wrote:
 >
 >> From: Andrew Melnychenko 
 >>
 >> There is an issue, that netdev can't be removed if it was added
 using hmp.
 >> The bug appears after 08712fcb851034228b61f75bd922863a984a4f60
 commit.
 >> It happens because of unclear QemuOpts that was created during
 >> hmp_netdev_add(), now it uses qmp analog function -
 >> qmp_marshal_netdev_add().
 >>
 >> Signed-off-by: Andrew Melnychenko 
 >> ---
 >>  monitor/hmp-cmds.c | 15 +++
 >>  1 file changed, 3 insertions(+), 12 deletions(-)
 >>
 >> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
 >> index 2b0b58a336..b747935687 100644
 >> --- a/monitor/hmp-cmds.c
 >> +++ b/monitor/hmp-cmds.c
 >> @@ -1597,19 +1597,10 @@ void hmp_migrate(Monitor *mon, const QDict
 *qdict)
 >>  void hmp_netdev_add(Monitor *mon, const QDict *qdict)
 >>  {
 >>  Error *err = NULL;
 >> -QemuOpts *opts;
 >> -
 >> -opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict,
 );
 >> -if (err) {
 >> -goto out;
 >> -}
 >> +QDict *non_constant_dict = qdict_clone_shallow(qdict);
 >>
 >> -netdev_add(opts, );
 >> -if (err) {
 >> -qemu_opts_del(opts);
 >> -}
 >> -
 >> -out:
 >> +qmp_marshal_netdev_add(non_constant_dict, NULL, );
 >> +qobject_unref(non_constant_dict);
 >>  hmp_handle_error(mon, err);
 >>  }

 qmp_marshal_netdev_add() uses the QObject input visitor, which feels
 wrong for HMP input.

 What exactly is the problem you're trying to solve?  Can you show us a
 reproducer?

 The problem was found during work on hotplug/unplug problems with q35
>>> run q35 VM with netdev and hotpluggable nic (virtio or e1000e)
>>> unplug the nic (device_del)
>>> delete the netdev ()
>>> add netdev with the same id as before - fail (Duplicated ID)
>>>
>>> Q35 is not mandatory for reproduction, the same with '-machine pc'
>>
>>

[PULL 1/1] hw/display/tcx: add missing 64-bit access for framebuffer blitter

[Mark Cave-Ayland]

Commit ae5643ecc6 "hw/display/tcx: Allow 64-bit accesses to framebuffer stippler
and blitter" enabled 64-bit access for the TCX framebuffer stippler and blitter
but missed applying the change to one of the blitter MemoryRegions.

Whilst the original change works for me on my local NetBSD test image, the 
latest
NetBSD ISO panics on startup without this fix.

Signed-off-by: Mark Cave-Ayland 
Fixes: ae5643ecc6 ("hw/display/tcx: Allow 64-bit accesses to framebuffer 
stippler and blitter")
Buglink: https://bugs.launchpad.net/bugs/1892540
Message-Id: <20201120081754.18250-1-mark.cave-ayl...@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Mark Cave-Ayland 
---
 hw/display/tcx.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 878ecc8c50..3799d29b75 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -649,10 +649,14 @@ static const MemoryRegionOps tcx_blit_ops = {
 .read = tcx_blit_readl,
 .write = tcx_blit_writel,
 .endianness = DEVICE_NATIVE_ENDIAN,
-.valid = {
+.impl = {
 .min_access_size = 4,
 .max_access_size = 4,
 },
+.valid = {
+.min_access_size = 4,
+.max_access_size = 8,
+},
 };
 
 static const MemoryRegionOps tcx_rblit_ops = {
-- 
2.20.1

[PULL 0/1] qemu-sparc queue 20201122

[Mark Cave-Ayland]

The following changes since commit e3a232cccd2445e5d9e607a65a78cdbc33ff8a0f:

  Merge remote-tracking branch 'remotes/bonzini-gitlab/tags/for-upstream' into 
staging (2020-11-20 22:30:51 +)

are available in the Git repository at:

  git://github.com/mcayland/qemu.git tags/qemu-sparc-20201122

for you to fetch changes up to 48e5c7f34c557afe49396a00169629d24dc3342d:

  hw/display/tcx: add missing 64-bit access for framebuffer blitter (2020-11-22 
10:43:30 +)


qemu-sparc queue


Mark Cave-Ayland (1):
  hw/display/tcx: add missing 64-bit access for framebuffer blitter

 hw/display/tcx.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

Re: [PATCH 1/1] /net/tap.c: Fix a memory leak

[Peter Maydell]

On Sun, 22 Nov 2020 at 11:07,  wrote:
>
> From: yuanjungong 
>
> Close fd before returning.
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1904486
> Signed-off-by: yuanjungong 
> ---
>  net/tap.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/net/tap.c b/net/tap.c
> index c46ff66..fe95fa7 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -817,6 +817,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
>  if (ret < 0) {
>  error_setg_errno(errp, -ret, "%s: Can't use file descriptor %d",
>   name, fd);
> +close(fd);
>  return -1;
>  }
>
> @@ -831,6 +832,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
>   vhostfdname, vnet_hdr, fd, );
>  if (err) {
>  error_propagate(errp, err);
> +close(fd);
>  return -1;
>  }
>  } else if (tap->has_fds) {
> --
> 2.17.1

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [Bug 1892540] Re: qemu can no longer boot NetBSD/sparc

[Mark Cave-Ayland]

On 21/11/2020 23:46, Peter Maydell wrote:


Is this bug now fixed, or are there still more patches not yet in
master?


The additional for-5.2 patch above is still needed: I've just submitted it to 
Travis-CI, and assuming it passes I'll send a PR later.



ATB,

Mark.

[PATCH 1/1] /net/tap.c: Fix a memory leak

[ruc_gongyuanjun]

From: yuanjungong 

Close fd before returning.

Buglink: https://bugs.launchpad.net/qemu/+bug/1904486
Signed-off-by: yuanjungong 
---
 net/tap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/tap.c b/net/tap.c
index c46ff66..fe95fa7 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -817,6 +817,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
 if (ret < 0) {
 error_setg_errno(errp, -ret, "%s: Can't use file descriptor %d",
  name, fd);
+close(fd);
 return -1;
 }
 
@@ -831,6 +832,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
  vhostfdname, vnet_hdr, fd, );
 if (err) {
 error_propagate(errp, err);
+close(fd);
 return -1;
 }
 } else if (tap->has_fds) {
-- 
2.17.1

[PATCH v4 1/2] hw/misc: add an EMC141{3,4} device model

[John Wang]

Largely inspired by the TMP421 temperature sensor, here is a model for
the EMC1413/EMC1414 temperature sensors.

Specs can be found here :
  http://ww1.microchip.com/downloads/en/DeviceDoc/20005274A.pdf

Signed-off-by: John Wang 
---
v4:
  -Fix QOM style name
  -Add unittest
v3:
  - update the link to the spec
  - Rename emc1413.c to emc141x.c
  - Add sensors_count in EMC141XClass
  - Make emc1413_read/write easier to review :)
v2:
  - Remove DeviceInfo
  - commit message: TMP423 -> TMP421
---
 hw/arm/Kconfig |   1 +
 hw/misc/Kconfig|   4 +
 hw/misc/emc141x.c  | 326 +
 hw/misc/meson.build|   1 +
 include/hw/misc/emc141x_regs.h |  37 
 tests/qtest/emc141x-test.c |  81 
 tests/qtest/meson.build|   1 +
 7 files changed, 451 insertions(+)
 create mode 100644 hw/misc/emc141x.c
 create mode 100644 include/hw/misc/emc141x_regs.h
 create mode 100644 tests/qtest/emc141x-test.c

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index e69a9009cf..eb8a8844cf 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -407,6 +407,7 @@ config ASPEED_SOC
 select SSI_M25P80
 select TMP105
 select TMP421
+select EMC141X
 select UNIMP
 select LED
 
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index dc44dc14f6..cf18ac08e6 100644
--- a/hw/misc/Kconfig
+++ b/hw/misc/Kconfig
@@ -13,6 +13,10 @@ config TMP421
 bool
 depends on I2C
 
+config EMC141X
+bool
+depends on I2C
+
 config ISA_DEBUG
 bool
 depends on ISA_BUS
diff --git a/hw/misc/emc141x.c b/hw/misc/emc141x.c
new file mode 100644
index 00..f7c53d48a4
--- /dev/null
+++ b/hw/misc/emc141x.c
@@ -0,0 +1,326 @@
+/*
+ * SMSC EMC141X temperature sensor.
+ *
+ * Copyright (c) 2020 Bytedance Corporation
+ * Written by John Wang 
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 or
+ * (at your option) version 3 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see .
+ */
+
+#include "qemu/osdep.h"
+#include "hw/i2c/i2c.h"
+#include "migration/vmstate.h"
+#include "qapi/error.h"
+#include "qapi/visitor.h"
+#include "qemu/module.h"
+#include "qom/object.h"
+#include "hw/misc/emc141x_regs.h"
+
+#define SENSORS_COUNT_MAX4
+
+struct EMC141XState {
+I2CSlave parent_obj;
+struct {
+uint8_t raw_temp_min;
+uint8_t raw_temp_current;
+uint8_t raw_temp_max;
+} sensor[SENSORS_COUNT_MAX];
+uint8_t len;
+uint8_t data;
+uint8_t pointer;
+};
+
+struct EMC141XClass {
+I2CSlaveClass parent_class;
+uint8_t model;
+unsigned sensors_count;
+};
+
+#define TYPE_EMC141X "emc141x"
+OBJECT_DECLARE_TYPE(EMC141XState, EMC141XClass, EMC141X)
+
+static void emc141x_get_temperature(Object *obj, Visitor *v, const char *name,
+void *opaque, Error **errp)
+{
+EMC141XState *s = EMC141X(obj);
+EMC141XClass *sc = EMC141X_GET_CLASS(s);
+int64_t value;
+unsigned tempid;
+
+if (sscanf(name, "temperature%u", ) != 1) {
+error_setg(errp, "error reading %s: %s", name, g_strerror(errno));
+return;
+}
+
+if (tempid >= sc->sensors_count) {
+error_setg(errp, "error reading %s", name);
+return;
+}
+
+value = s->sensor[tempid].raw_temp_current * 1000;
+
+visit_type_int(v, name, , errp);
+}
+
+static void emc141x_set_temperature(Object *obj, Visitor *v, const char *name,
+void *opaque, Error **errp)
+{
+EMC141XState *s = EMC141X(obj);
+EMC141XClass *sc = EMC141X_GET_CLASS(s);
+int64_t temp;
+unsigned tempid;
+
+if (!visit_type_int(v, name, , errp)) {
+return;
+}
+
+if (sscanf(name, "temperature%u", ) != 1) {
+error_setg(errp, "error reading %s: %s", name, g_strerror(errno));
+return;
+}
+
+if (tempid >= sc->sensors_count) {
+error_setg(errp, "error reading %s", name);
+return;
+}
+
+s->sensor[tempid].raw_temp_current = temp / 1000;
+}
+
+static void emc141x_read(EMC141XState *s)
+{
+EMC141XClass *sc = EMC141X_GET_CLASS(s);
+switch (s->pointer) {
+case EMC141X_DEVICE_ID:
+s->data = sc->model;
+break;
+case EMC141X_MANUFACTURER_ID:
+s->data = MANUFACTURER_ID;
+break;
+case EMC141X_REVISION:
+s->data = REVISION;
+break;
+case EMC141X_TEMP_HIGH0:
+s->data = s->sensor[0].raw_temp_current;
+break;
+case 

[PATCH v4 2/2] aspeed: Add support for the g220a-bmc board

[John Wang]

G220A is a 2 socket x86 motherboard supported by OpenBMC.
Strapping configuration was obtained from hardware.

Signed-off-by: John Wang 
Reviewed-by: Cédric Le Goater 
Reviewed-by: Joel Stanley 
---
v4:
  - No changes
v3:
  - No changes
v2:
  - No changes
---
 hw/arm/aspeed.c | 60 +
 1 file changed, 60 insertions(+)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 0ef3f6b412..aee00ba8d6 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -121,6 +121,20 @@ struct AspeedMachineState {
 SCU_AST2500_HW_STRAP_ACPI_ENABLE |  \
 SCU_HW_STRAP_SPI_MODE(SCU_HW_STRAP_SPI_MASTER))
 
+#define G220A_BMC_HW_STRAP1 (  \
+SCU_AST2500_HW_STRAP_SPI_AUTOFETCH_ENABLE | \
+SCU_AST2500_HW_STRAP_GPIO_STRAP_ENABLE |\
+SCU_AST2500_HW_STRAP_UART_DEBUG |   \
+SCU_AST2500_HW_STRAP_RESERVED28 |   \
+SCU_AST2500_HW_STRAP_DDR4_ENABLE |  \
+SCU_HW_STRAP_2ND_BOOT_WDT | \
+SCU_HW_STRAP_VGA_CLASS_CODE |   \
+SCU_HW_STRAP_LPC_RESET_PIN |\
+SCU_HW_STRAP_SPI_MODE(SCU_HW_STRAP_SPI_MASTER) |\
+SCU_AST2500_HW_STRAP_SET_AXI_AHB_RATIO(AXI_AHB_RATIO_2_1) | \
+SCU_HW_STRAP_VGA_SIZE_SET(VGA_64M_DRAM) |   \
+SCU_AST2500_HW_STRAP_RESERVED1)
+
 /* Witherspoon hardware value: 0xF10AD216 (but use romulus definition) */
 #define WITHERSPOON_BMC_HW_STRAP1 ROMULUS_BMC_HW_STRAP1
 
@@ -579,6 +593,30 @@ static void witherspoon_bmc_i2c_init(AspeedMachineState 
*bmc)
 /* Bus 11: TODO ucd90160@64 */
 }
 
+static void g220a_bmc_i2c_init(AspeedMachineState *bmc)
+{
+AspeedSoCState *soc = >soc;
+DeviceState *dev;
+
+dev = DEVICE(i2c_slave_create_simple(aspeed_i2c_get_bus(>i2c, 3),
+ "emc1413", 0x4c));
+object_property_set_int(OBJECT(dev), "temperature0", 31000, _abort);
+object_property_set_int(OBJECT(dev), "temperature1", 28000, _abort);
+object_property_set_int(OBJECT(dev), "temperature2", 2, _abort);
+
+dev = DEVICE(i2c_slave_create_simple(aspeed_i2c_get_bus(>i2c, 12),
+ "emc1413", 0x4c));
+object_property_set_int(OBJECT(dev), "temperature0", 31000, _abort);
+object_property_set_int(OBJECT(dev), "temperature1", 28000, _abort);
+object_property_set_int(OBJECT(dev), "temperature2", 2, _abort);
+
+dev = DEVICE(i2c_slave_create_simple(aspeed_i2c_get_bus(>i2c, 13),
+ "emc1413", 0x4c));
+object_property_set_int(OBJECT(dev), "temperature0", 31000, _abort);
+object_property_set_int(OBJECT(dev), "temperature1", 28000, _abort);
+object_property_set_int(OBJECT(dev), "temperature2", 2, _abort);
+}
+
 static bool aspeed_get_mmio_exec(Object *obj, Error **errp)
 {
 return ASPEED_MACHINE(obj)->mmio_exec;
@@ -818,6 +856,24 @@ static void aspeed_machine_tacoma_class_init(ObjectClass 
*oc, void *data)
 aspeed_soc_num_cpus(amc->soc_name);
 };
 
+static void aspeed_machine_g220a_class_init(ObjectClass *oc, void *data)
+{
+MachineClass *mc = MACHINE_CLASS(oc);
+AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
+
+mc->desc   = "Bytedance G220A BMC (ARM1176)";
+amc->soc_name  = "ast2500-a1";
+amc->hw_strap1 = G220A_BMC_HW_STRAP1;
+amc->fmc_model = "n25q512a";
+amc->spi_model = "mx25l25635e";
+amc->num_cs= 2;
+amc->macs_mask  = ASPEED_MAC1_ON | ASPEED_MAC2_ON;
+amc->i2c_init  = g220a_bmc_i2c_init;
+mc->default_ram_size = 1024 * MiB;
+mc->default_cpus = mc->min_cpus = mc->max_cpus =
+aspeed_soc_num_cpus(amc->soc_name);
+};
+
 static const TypeInfo aspeed_machine_types[] = {
 {
 .name  = MACHINE_TYPE_NAME("palmetto-bmc"),
@@ -855,6 +911,10 @@ static const TypeInfo aspeed_machine_types[] = {
 .name  = MACHINE_TYPE_NAME("tacoma-bmc"),
 .parent= TYPE_ASPEED_MACHINE,
 .class_init= aspeed_machine_tacoma_class_init,
+}, {
+.name  = MACHINE_TYPE_NAME("g220a-bmc"),
+.parent= TYPE_ASPEED_MACHINE,
+.class_init= aspeed_machine_g220a_class_init,
 }, {
 .name  = TYPE_ASPEED_MACHINE,
 .parent= TYPE_MACHINE,
-- 
2.25.1

Re: [Phishing Risk] [External] Re: [PATCH v3 1/2] hw/misc: add an EMC141{3,4} device model

[John Wang]

On Mon, Oct 26, 2020 at 5:08 AM Philippe Mathieu-Daudé  wrote:
>
> On 10/25/20 2:14 PM, John Wang wrote:
> > Largely inspired by the TMP421 temperature sensor, here is a model for
> > the EMC1413/EMC1414 temperature sensors.
> >
> > Specs can be found here :
> >http://ww1.microchip.com/downloads/en/DeviceDoc/20005274A.pdf
> >
> > Signed-off-by: John Wang 
> > ---
> > v3:
> >- update the link to the spec
> >- Rename emc1413.c to emc141x.c
> >- Add sensors_count in EMC141XClass
> >- Make emc1413_read/write easier to review :)
>
> Thanks for the update.
>
> > v2:
> >- Remove DeviceInfo
> >- commit message: TMP423 -> TMP421
> > ---
> >   hw/arm/Kconfig  |   1 +
> >   hw/misc/Kconfig |   4 +
> >   hw/misc/emc141x.c   | 347 
> >   hw/misc/meson.build |   1 +
> >   4 files changed, 353 insertions(+)
> >   create mode 100644 hw/misc/emc141x.c
> >
> > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> > index 7d040827af..3dd651a236 100644
> > --- a/hw/arm/Kconfig
> > +++ b/hw/arm/Kconfig
> > @@ -403,6 +403,7 @@ config ASPEED_SOC
> >   select SSI_M25P80
> >   select TMP105
> >   select TMP421
> > +select EMC141X
> >   select UNIMP
> >
> >   config MPS2
> > diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
> > index 3185456110..169d087d3d 100644
> > --- a/hw/misc/Kconfig
> > +++ b/hw/misc/Kconfig
> > @@ -13,6 +13,10 @@ config TMP421
> >   bool
> >   depends on I2C
> >
> > +config EMC141X
> > +bool
> > +depends on I2C
> > +
> >   config ISA_DEBUG
> >   bool
> >   depends on ISA_BUS
> > diff --git a/hw/misc/emc141x.c b/hw/misc/emc141x.c
> > new file mode 100644
> > index 00..a2bce74719
> > --- /dev/null
> > +++ b/hw/misc/emc141x.c
> > @@ -0,0 +1,347 @@
> > +/*
> > + * SMSC EMC141X temperature sensor.
> > + *
> > + * Copyright (c) 2020 Bytedance Corporation
> > + * Written by John Wang 
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 or
> > + * (at your option) version 3 of the License.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License along
> > + * with this program; if not, see .
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "hw/i2c/i2c.h"
> > +#include "migration/vmstate.h"
> > +#include "qapi/error.h"
> > +#include "qapi/visitor.h"
> > +#include "qemu/module.h"
> > +#include "qom/object.h"
> > +
> > +#define EMC1413_DEVICE_ID0x21
> > +#define EMC1414_DEVICE_ID0x25
> > +#define MANUFACTURER_ID  0x5d
> > +#define REVISION 0x04
> > +
> > +#define SENSORS_COUNT_MAX4
> > +
> > +struct EMC141XState {
> > +I2CSlave i2c;
>
> The QOM style name is 'parent_obj':
>
> I2CSlave parent_obj;

will fix

>
> > +uint8_t temperature[SENSORS_COUNT_MAX];
>
> What is the unit?
>
> > +uint8_t min[SENSORS_COUNT_MAX];
> > +uint8_t max[SENSORS_COUNT_MAX];
>
> min/max what? Temperature?
>
> What about (if the unit is milli Celsius):
>
> struct {
> uint8_t temp_min_mC;
> uint8_t temp_current_mC;
> uint8_t temp_max_mC;
> } sensor[SENSORS_COUNT_MAX];
>
> Looking at the datasheet chapter 6.8 "Temperature Measurement
> Results  and  Data", it isn't in milli Celsius. See below in
> emc141x_get_temperature().
>
> So maybe:
>
> struct {
> uint8_t raw_temp_min;
> uint8_t raw_temp_current;
> uint8_t raw_temp_max;
> } sensor[SENSORS_COUNT_MAX];

will fix

>
> > +uint8_t len;
> > +uint8_t data;
> > +uint8_t pointer;
> > +};
> > +
> > +struct EMC141XClass {
> > +I2CSlaveClass parent_class;
> > +uint8_t model;
> > +unsigned sensors_count;
> > +};
> > +
> > +#define TYPE_EMC141X "emc141x"
> > +OBJECT_DECLARE_TYPE(EMC141XState, EMC141XClass, EMC141X)
> > +
> > +
> > +/* the EMC141X registers */
> > +#define EMC141X_TEMP_HIGH0   0x00
> > +#define EMC141X_TEMP_HIGH1   0x01
> > +#define EMC141X_TEMP_HIGH2   0x23
> > +#define EMC141X_TEMP_HIGH3   0x2a
> > +#define EMC141X_TEMP_MAX_HIGH0   0x05
> > +#define EMC141X_TEMP_MIN_HIGH0   0x06
> > +#define EMC141X_TEMP_MAX_HIGH1   0x07
> > +#define EMC141X_TEMP_MIN_HIGH1   0x08
> > +#define EMC141X_TEMP_MAX_HIGH2   0x15
> > +#define EMC141X_TEMP_MIN_HIGH2   0x16
> > +#define EMC141X_TEMP_MAX_HIGH3   0x2c
> > +#define EMC141X_TEMP_MIN_HIGH3   0x2d
> > 

Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[Andrew Melnichenko]

Hi, the bug can be reproduced like that:

> QEMU 5.1.50 monitor - type 'help' for more information
> (qemu) netdev_add
> type=tap,id=net0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no
> (qemu) info network
> hub 0
>  \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off
>  \ hub0port0: e1000e.0:
> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56
> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
> net0:
> index=0,type=tap,ifname=tap0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no
> (qemu) netdev_del net0
> (qemu) info network
> hub 0
>  \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off
>  \ hub0port0: e1000e.0:
> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56
> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
> (qemu) netdev_add
> type=tap,id=net0,script=/home/and/SRCS/qemu/ifup.sh,downscript=no
> Try "help netdev_add" for more information
> (qemu) info network
> hub 0
>  \ hub0port1: __org.qemu.net1: index=0,type=user,net=10.0.2.0,restrict=off
>  \ hub0port0: e1000e.0:
> index=0,type=nic,model=e1000e,macaddr=52:54:00:12:34:56
> dnet0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:57
> (qemu)
>
>
Its still actual bug - I've checked it with the
master(2c6605389c1f76973d92b69b85d40d94b8f1092c).

On Sat, Nov 21, 2020 at 5:31 PM Yuri Benditovich <
yuri.benditov...@daynix.com> wrote:

>
>
> On Sat, Nov 21, 2020 at 5:24 PM Yuri Benditovich <
> yuri.benditov...@daynix.com> wrote:
>
>>
>>
>> On Fri, Nov 20, 2020 at 2:58 PM Markus Armbruster 
>> wrote:
>>
>>> Andrew Melnichenko  writes:
>>>
>>> > Ping
>>> >
>>> > On Thu, Jul 16, 2020 at 6:26 AM  wrote:
>>> >
>>> >> From: Andrew Melnychenko 
>>> >>
>>> >> There is an issue, that netdev can't be removed if it was added using
>>> hmp.
>>> >> The bug appears after 08712fcb851034228b61f75bd922863a984a4f60 commit.
>>> >> It happens because of unclear QemuOpts that was created during
>>> >> hmp_netdev_add(), now it uses qmp analog function -
>>> >> qmp_marshal_netdev_add().
>>> >>
>>> >> Signed-off-by: Andrew Melnychenko 
>>> >> ---
>>> >>  monitor/hmp-cmds.c | 15 +++
>>> >>  1 file changed, 3 insertions(+), 12 deletions(-)
>>> >>
>>> >> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>>> >> index 2b0b58a336..b747935687 100644
>>> >> --- a/monitor/hmp-cmds.c
>>> >> +++ b/monitor/hmp-cmds.c
>>> >> @@ -1597,19 +1597,10 @@ void hmp_migrate(Monitor *mon, const QDict
>>> *qdict)
>>> >>  void hmp_netdev_add(Monitor *mon, const QDict *qdict)
>>> >>  {
>>> >>  Error *err = NULL;
>>> >> -QemuOpts *opts;
>>> >> -
>>> >> -opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict,
>>> );
>>> >> -if (err) {
>>> >> -goto out;
>>> >> -}
>>> >> +QDict *non_constant_dict = qdict_clone_shallow(qdict);
>>> >>
>>> >> -netdev_add(opts, );
>>> >> -if (err) {
>>> >> -qemu_opts_del(opts);
>>> >> -}
>>> >> -
>>> >> -out:
>>> >> +qmp_marshal_netdev_add(non_constant_dict, NULL, );
>>> >> +qobject_unref(non_constant_dict);
>>> >>  hmp_handle_error(mon, err);
>>> >>  }
>>>
>>> qmp_marshal_netdev_add() uses the QObject input visitor, which feels
>>> wrong for HMP input.
>>>
>>> What exactly is the problem you're trying to solve?  Can you show us a
>>> reproducer?
>>>
>>> The problem was found during work on hotplug/unplug problems with q35
>> run q35 VM with netdev and hotpluggable nic (virtio or e1000e)
>> unplug the nic (device_del)
>> delete the netdev ()
>> add netdev with the same id as before - fail (Duplicated ID)
>>
>> Q35 is not mandatory for reproduction, the same with '-machine pc'
>
>