[PATCH] pci: add enforce_slot_reserved_mask_manual property

[PATCH] pci: add enforce_slot_reserved_mask_manual property

[Chuck Zmudzinski]

The current reserved slot check in do_pci_register_device(), added with
commit 8b8849844fd6, is done even if the pci device being added is
configured manually for a particular slot. The new property, when set
to false, disables the check when the device is configured to request a
particular slot. This allows an administrator or management tool to
override slot_reserved_mask for a pci device by requesting a particular
slot for the device. The new property is initialized to true which
preserves the existing behavior of slot_reserved_mask by default.

Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
---
 hw/pci/pci.c             | 9 ++++++++-
 include/hw/pci/pci_bus.h | 1 +
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index c2fb88f9a3..5e15f08036 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -467,6 +467,7 @@ static void pci_root_bus_internal_init(PCIBus *bus, DeviceState *parent,
     assert(PCI_FUNC(devfn_min) == 0);
     bus->devfn_min = devfn_min;
     bus->slot_reserved_mask = 0x0;
+    bus->enforce_slot_reserved_mask_manual = true;
     bus->address_space_mem = address_space_mem;
     bus->address_space_io = address_space_io;
     bus->flags |= PCI_BUS_IS_ROOT;
@@ -1074,6 +1075,12 @@ static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn)
     return bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn));
 }
 
+static bool pci_bus_devfn_reserved_manual(PCIBus *bus, int devfn)
+{
+    return bus->enforce_slot_reserved_mask_manual &&
+            (bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn)));
+}
+
 /* -1 for devfn means auto assign */
 static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
                                          const char *name, int devfn,
@@ -1107,7 +1114,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
                    "or reserved", name);
         return NULL;
     found: ;
-    } else if (pci_bus_devfn_reserved(bus, devfn)) {
+    } else if (pci_bus_devfn_reserved_manual(bus, devfn)) {
         error_setg(errp, "PCI: slot %d function %d not available for %s,"
                    " reserved",
                    PCI_SLOT(devfn), PCI_FUNC(devfn), name);
diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index 5653175957..e0f15ee9be 100644
--- a/include/hw/pci/pci_bus.h
+++ b/include/hw/pci/pci_bus.h
@@ -37,6 +37,7 @@ struct PCIBus {
     void *iommu_opaque;
     uint8_t devfn_min;
     uint32_t slot_reserved_mask;
+    bool enforce_slot_reserved_mask_manual;
     pci_set_irq_fn set_irq;
     pci_map_irq_fn map_irq;
     pci_route_irq_fn route_intx_to_irq;
-- 
2.39.0

Re: [PATCH] pci: add enforce_slot_reserved_mask_manual property

[Michael S. Tsirkin]

On Sun, Jan 15, 2023 at 07:49:51PM -0500, Chuck Zmudzinski wrote:
> The current reserved slot check in do_pci_register_device(), added with
> commit 8b8849844fd6

add ("subject here") please

> ,is done even if the pci device being added is
> configured manually for a particular slot. The new property, when set
> to false, disables the check when the device is configured to request a
> particular slot. This allows an administrator or management tool to
> override slot_reserved_mask for a pci device by requesting a particular
> slot for the device. The new property is initialized to true which
> preserves the existing behavior of slot_reserved_mask by default.
> 
> Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>

Thanks!
I'm trying to think of the best default for this.
Users is trying to configure a specific device on a reserved
slot. Should we 
CC a bunch more people for visibility. Input, anyone?


> ---
>  hw/pci/pci.c             | 9 ++++++++-
>  include/hw/pci/pci_bus.h | 1 +
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index c2fb88f9a3..5e15f08036 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -467,6 +467,7 @@ static void pci_root_bus_internal_init(PCIBus *bus, DeviceState *parent,
>      assert(PCI_FUNC(devfn_min) == 0);
>      bus->devfn_min = devfn_min;
>      bus->slot_reserved_mask = 0x0;
> +    bus->enforce_slot_reserved_mask_manual = true;
>      bus->address_space_mem = address_space_mem;
>      bus->address_space_io = address_space_io;
>      bus->flags |= PCI_BUS_IS_ROOT;
> @@ -1074,6 +1075,12 @@ static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn)
>      return bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn));
>  }
>  
> +static bool pci_bus_devfn_reserved_manual(PCIBus *bus, int devfn)
> +{
> +    return bus->enforce_slot_reserved_mask_manual &&
> +            (bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn)));
> +}
> +
>  /* -1 for devfn means auto assign */
>  static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
>                                           const char *name, int devfn,
> @@ -1107,7 +1114,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
>                     "or reserved", name);
>          return NULL;
>      found: ;
> -    } else if (pci_bus_devfn_reserved(bus, devfn)) {
> +    } else if (pci_bus_devfn_reserved_manual(bus, devfn)) {
>          error_setg(errp, "PCI: slot %d function %d not available for %s,"
>                     " reserved",
>                     PCI_SLOT(devfn), PCI_FUNC(devfn), name);

Should this be a device property or a bus property?
And maybe now mention the property name so users know how
to override this?

> diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> index 5653175957..e0f15ee9be 100644
> --- a/include/hw/pci/pci_bus.h
> +++ b/include/hw/pci/pci_bus.h
> @@ -37,6 +37,7 @@ struct PCIBus {
>      void *iommu_opaque;
>      uint8_t devfn_min;
>      uint32_t slot_reserved_mask;
> +    bool enforce_slot_reserved_mask_manual;
>      pci_set_irq_fn set_irq;
>      pci_map_irq_fn map_irq;
>      pci_route_irq_fn route_intx_to_irq;
> -- 
> 2.39.0

Re: [PATCH] pci: add enforce_slot_reserved_mask_manual property

[Chuck Zmudzinski]

On 1/27/2023 8:28 AM, Michael S. Tsirkin wrote:
> On Sun, Jan 15, 2023 at 07:49:51PM -0500, Chuck Zmudzinski wrote:
> > The current reserved slot check in do_pci_register_device(), added with
> > commit 8b8849844fd6
>
> add ("subject here") please
>
> > ,is done even if the pci device being added is
> > configured manually for a particular slot. The new property, when set
> > to false, disables the check when the device is configured to request a
> > particular slot. This allows an administrator or management tool to
> > override slot_reserved_mask for a pci device by requesting a particular
> > slot for the device. The new property is initialized to true which
> > preserves the existing behavior of slot_reserved_mask by default.
> > 
> > Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
>
> Thanks!
> I'm trying to think of the best default for this.

I think it would be better for the default value of
enforce_slot_reserved_mask_manual to be false, so that a
user-specified slot will by default override slot_reserved_mask.
But doing that would change the current behavior of
slot_reserved_mask.

Currently, this is the only place where slot_reserved_mask is used in all
of the Qemu source (code from hw/sparc64/sun4u.c):

------ snip -------
    /* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is
       reserved (leaving no slots free after on-board devices) however slots
       0-3 are free on busB */
    pci_bus->slot_reserved_mask = 0xfffffffc;
    pci_busA->slot_reserved_mask = 0xfffffff1;
    pci_busB->slot_reserved_mask = 0xfffffff0;
------ snip -------

I think we could safely change the default value of
enforce_slot_reserved_mask_manual to false but set
it to true for the sparc64 sun4u board here to preserve
the current behavior of the only existing board in Qemu
that uses slot_reserved_mask.

What do you think?

> Users is trying to configure a specific device on a reserved
> slot. Should we 
> CC a bunch more people for visibility. Input, anyone?
>
>
> > ---
> >  hw/pci/pci.c             | 9 ++++++++-
> >  include/hw/pci/pci_bus.h | 1 +
> >  2 files changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index c2fb88f9a3..5e15f08036 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -467,6 +467,7 @@ static void pci_root_bus_internal_init(PCIBus *bus, DeviceState *parent,
> >      assert(PCI_FUNC(devfn_min) == 0);
> >      bus->devfn_min = devfn_min;
> >      bus->slot_reserved_mask = 0x0;
> > +    bus->enforce_slot_reserved_mask_manual = true;
> >      bus->address_space_mem = address_space_mem;
> >      bus->address_space_io = address_space_io;
> >      bus->flags |= PCI_BUS_IS_ROOT;
> > @@ -1074,6 +1075,12 @@ static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn)
> >      return bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn));
> >  }
> >  
> > +static bool pci_bus_devfn_reserved_manual(PCIBus *bus, int devfn)
> > +{
> > +    return bus->enforce_slot_reserved_mask_manual &&
> > +            (bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn)));
> > +}
> > +
> >  /* -1 for devfn means auto assign */
> >  static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
> >                                           const char *name, int devfn,
> > @@ -1107,7 +1114,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
> >                     "or reserved", name);
> >          return NULL;
> >      found: ;
> > -    } else if (pci_bus_devfn_reserved(bus, devfn)) {
> > +    } else if (pci_bus_devfn_reserved_manual(bus, devfn)) {
> >          error_setg(errp, "PCI: slot %d function %d not available for %s,"
> >                     " reserved",
> >                     PCI_SLOT(devfn), PCI_FUNC(devfn), name);
>
> Should this be a device property or a bus property?
> And maybe now mention the property name so users know how
> to override this?

If we implement the change to make the default value of
enforce_slot_reserved_mask_manual false, the only  case
when the user will not be able to override it simply by specifying
a particular slot would be when it is a user of the sparc64 sun4u
board who wants to put a device in a slot that board does not
allow. In that case, it would be nice to report an error here that
explains why the slot is reserved even with a manual configuration
of the slot. I could add another property to the bus which will allow
for providing a reason the slot is reserved that can be used in the
error message instead of the current generic message that says the
slot is reserved without explaining why.

I will also think about overriding slot_reserved_mask using
a device property instead of a bus property, but I think keeping
it as a bus property, making it false by default, and adding a
reason for having reserved slots even with manually configured
slots (as in the case of the sparc64 sun4u board) in the form of
an error message string would improve the error message here
without changing the current behavior of the sparc64 sun4u board
and make the slot_reserved_mask useful for the case when it is
necessary to reserve a slot for a particular device when device slots
are auto-assigned but it is not necessary to reserve the slot when
the slot addresses are manually assigned. Since there is only the one
case in Qemu (the sparc64 sun4u board) where a specific error message
would be needed to be added, it is not too much work to implement this
change.

Does that make sense?

> > diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> > index 5653175957..e0f15ee9be 100644
> > --- a/include/hw/pci/pci_bus.h
> > +++ b/include/hw/pci/pci_bus.h
> > @@ -37,6 +37,7 @@ struct PCIBus {
> >      void *iommu_opaque;
> >      uint8_t devfn_min;
> >      uint32_t slot_reserved_mask;
> > +    bool enforce_slot_reserved_mask_manual;
> >      pci_set_irq_fn set_irq;
> >      pci_map_irq_fn map_irq;
> >      pci_route_irq_fn route_intx_to_irq;
> > -- 
> > 2.39.0
>

Re: [PATCH] pci: add enforce_slot_reserved_mask_manual property

[Michael S. Tsirkin]

On Fri, Jan 27, 2023 at 10:39:28PM -0500, Chuck Zmudzinski wrote:
> On 1/27/2023 8:28 AM, Michael S. Tsirkin wrote:
> > On Sun, Jan 15, 2023 at 07:49:51PM -0500, Chuck Zmudzinski wrote:
> > > The current reserved slot check in do_pci_register_device(), added with
> > > commit 8b8849844fd6
> >
> > add ("subject here") please
> >
> > > ,is done even if the pci device being added is
> > > configured manually for a particular slot. The new property, when set
> > > to false, disables the check when the device is configured to request a
> > > particular slot. This allows an administrator or management tool to
> > > override slot_reserved_mask for a pci device by requesting a particular
> > > slot for the device. The new property is initialized to true which
> > > preserves the existing behavior of slot_reserved_mask by default.
> > > 
> > > Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
> >
> > Thanks!
> > I'm trying to think of the best default for this.
> 
> I think it would be better for the default value of
> enforce_slot_reserved_mask_manual to be false, so that a
> user-specified slot will by default override slot_reserved_mask.
> But doing that would change the current behavior of
> slot_reserved_mask.
> 
> Currently, this is the only place where slot_reserved_mask is used in all
> of the Qemu source (code from hw/sparc64/sun4u.c):
> 
> ------ snip -------
>     /* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is
>        reserved (leaving no slots free after on-board devices) however slots
>        0-3 are free on busB */
>     pci_bus->slot_reserved_mask = 0xfffffffc;
>     pci_busA->slot_reserved_mask = 0xfffffff1;
>     pci_busB->slot_reserved_mask = 0xfffffff0;
> ------ snip -------
> 
> I think we could safely change the default value of
> enforce_slot_reserved_mask_manual to false but set
> it to true for the sparc64 sun4u board here to preserve
> the current behavior of the only existing board in Qemu
> that uses slot_reserved_mask.
> 
> What do you think?

I guess first can you answer whether this is still needed
with the latest Xen patches?

-- 
MST

Re: [PATCH] pci: add enforce_slot_reserved_mask_manual property

[Chuck Zmudzinski]

On 1/28/23 5:26 AM, Michael S. Tsirkin wrote:
> On Fri, Jan 27, 2023 at 10:39:28PM -0500, Chuck Zmudzinski wrote:
>> On 1/27/2023 8:28 AM, Michael S. Tsirkin wrote:
>> > On Sun, Jan 15, 2023 at 07:49:51PM -0500, Chuck Zmudzinski wrote:
>> > > The current reserved slot check in do_pci_register_device(), added with
>> > > commit 8b8849844fd6
>> >
>> > add ("subject here") please
>> >
>> > > ,is done even if the pci device being added is
>> > > configured manually for a particular slot. The new property, when set
>> > > to false, disables the check when the device is configured to request a
>> > > particular slot. This allows an administrator or management tool to
>> > > override slot_reserved_mask for a pci device by requesting a particular
>> > > slot for the device. The new property is initialized to true which
>> > > preserves the existing behavior of slot_reserved_mask by default.
>> > > 
>> > > Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
>> >
>> > Thanks!
>> > I'm trying to think of the best default for this.
>> 
>> I think it would be better for the default value of
>> enforce_slot_reserved_mask_manual to be false, so that a
>> user-specified slot will by default override slot_reserved_mask.
>> But doing that would change the current behavior of
>> slot_reserved_mask.
>> 
>> Currently, this is the only place where slot_reserved_mask is used in all
>> of the Qemu source (code from hw/sparc64/sun4u.c):
>> 
>> ------ snip -------
>>     /* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is
>>        reserved (leaving no slots free after on-board devices) however slots
>>        0-3 are free on busB */
>>     pci_bus->slot_reserved_mask = 0xfffffffc;
>>     pci_busA->slot_reserved_mask = 0xfffffff1;
>>     pci_busB->slot_reserved_mask = 0xfffffff0;
>> ------ snip -------
>> 
>> I think we could safely change the default value of
>> enforce_slot_reserved_mask_manual to false but set
>> it to true for the sparc64 sun4u board here to preserve
>> the current behavior of the only existing board in Qemu
>> that uses slot_reserved_mask.
>> 
>> What do you think?
> 
> I guess first can you answer whether this is still needed
> with the latest Xen patches?
> 

It's not really needed except for experimental purposes to allow
an administrator to test experimental configurations with a device
other than the igd at slot 2. That might be useful in some cases,
but it is not really necessary unless someone asks for that capability.
If libvirt users who ordinarily like to manually specify all the
settings will be OK with the proposed patch to xen that prevents
an administrator from being able to override a new setting that
reserves slot 2 for the igd for type "xenfv" machines configured for
igd passthrough, then there is no need for this patch. I don't think
many users need the capability to insert a different device in slot 2 for
the "xenfv" machine type configured with igd-passthru=on, so I would be
OK if this patch is not included in qemu.

Chuck

Re: [PATCH] pci: add enforce_slot_reserved_mask_manual property

[Michael S. Tsirkin]

On Sat, Jan 28, 2023 at 08:20:55AM -0500, Chuck Zmudzinski wrote:
> On 1/28/23 5:26 AM, Michael S. Tsirkin wrote:
> > On Fri, Jan 27, 2023 at 10:39:28PM -0500, Chuck Zmudzinski wrote:
> >> On 1/27/2023 8:28 AM, Michael S. Tsirkin wrote:
> >> > On Sun, Jan 15, 2023 at 07:49:51PM -0500, Chuck Zmudzinski wrote:
> >> > > The current reserved slot check in do_pci_register_device(), added with
> >> > > commit 8b8849844fd6
> >> >
> >> > add ("subject here") please
> >> >
> >> > > ,is done even if the pci device being added is
> >> > > configured manually for a particular slot. The new property, when set
> >> > > to false, disables the check when the device is configured to request a
> >> > > particular slot. This allows an administrator or management tool to
> >> > > override slot_reserved_mask for a pci device by requesting a particular
> >> > > slot for the device. The new property is initialized to true which
> >> > > preserves the existing behavior of slot_reserved_mask by default.
> >> > > 
> >> > > Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
> >> >
> >> > Thanks!
> >> > I'm trying to think of the best default for this.
> >> 
> >> I think it would be better for the default value of
> >> enforce_slot_reserved_mask_manual to be false, so that a
> >> user-specified slot will by default override slot_reserved_mask.
> >> But doing that would change the current behavior of
> >> slot_reserved_mask.
> >> 
> >> Currently, this is the only place where slot_reserved_mask is used in all
> >> of the Qemu source (code from hw/sparc64/sun4u.c):
> >> 
> >> ------ snip -------
> >>     /* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is
> >>        reserved (leaving no slots free after on-board devices) however slots
> >>        0-3 are free on busB */
> >>     pci_bus->slot_reserved_mask = 0xfffffffc;
> >>     pci_busA->slot_reserved_mask = 0xfffffff1;
> >>     pci_busB->slot_reserved_mask = 0xfffffff0;
> >> ------ snip -------
> >> 
> >> I think we could safely change the default value of
> >> enforce_slot_reserved_mask_manual to false but set
> >> it to true for the sparc64 sun4u board here to preserve
> >> the current behavior of the only existing board in Qemu
> >> that uses slot_reserved_mask.
> >> 
> >> What do you think?
> > 
> > I guess first can you answer whether this is still needed
> > with the latest Xen patches?
> > 
> 
> It's not really needed except for experimental purposes to allow
> an administrator to test experimental configurations with a device
> other than the igd at slot 2. That might be useful in some cases,
> but it is not really necessary unless someone asks for that capability.
> If libvirt users who ordinarily like to manually specify all the
> settings will be OK with the proposed patch to xen that prevents
> an administrator from being able to override a new setting that
> reserves slot 2 for the igd for type "xenfv" machines configured for
> igd passthrough, then there is no need for this patch. I don't think
> many users need the capability to insert a different device in slot 2 for
> the "xenfv" machine type configured with igd-passthru=on, so I would be
> OK if this patch is not included in qemu.
> 
> Chuck

Pls wait and see if that patch gets picked up. Let me know.

Re: [PATCH] pci: add enforce_slot_reserved_mask_manual property

[Chuck Zmudzinski]

On 1/28/23 2:14 PM, Michael S. Tsirkin wrote:
> On Sat, Jan 28, 2023 at 08:20:55AM -0500, Chuck Zmudzinski wrote:
>> On 1/28/23 5:26 AM, Michael S. Tsirkin wrote:
>> > On Fri, Jan 27, 2023 at 10:39:28PM -0500, Chuck Zmudzinski wrote:
>> >> On 1/27/2023 8:28 AM, Michael S. Tsirkin wrote:
>> >> > On Sun, Jan 15, 2023 at 07:49:51PM -0500, Chuck Zmudzinski wrote:
>> >> > > The current reserved slot check in do_pci_register_device(), added with
>> >> > > commit 8b8849844fd6
>> >> >
>> >> > add ("subject here") please
>> >> >
>> >> > > ,is done even if the pci device being added is
>> >> > > configured manually for a particular slot. The new property, when set
>> >> > > to false, disables the check when the device is configured to request a
>> >> > > particular slot. This allows an administrator or management tool to
>> >> > > override slot_reserved_mask for a pci device by requesting a particular
>> >> > > slot for the device. The new property is initialized to true which
>> >> > > preserves the existing behavior of slot_reserved_mask by default.
>> >> > > 
>> >> > > Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
>> >> >
>> >> > Thanks!
>> >> > I'm trying to think of the best default for this.
>> >> 
>> >> I think it would be better for the default value of
>> >> enforce_slot_reserved_mask_manual to be false, so that a
>> >> user-specified slot will by default override slot_reserved_mask.
>> >> But doing that would change the current behavior of
>> >> slot_reserved_mask.
>> >> 
>> >> Currently, this is the only place where slot_reserved_mask is used in all
>> >> of the Qemu source (code from hw/sparc64/sun4u.c):
>> >> 
>> >> ------ snip -------
>> >>     /* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is
>> >>        reserved (leaving no slots free after on-board devices) however slots
>> >>        0-3 are free on busB */
>> >>     pci_bus->slot_reserved_mask = 0xfffffffc;
>> >>     pci_busA->slot_reserved_mask = 0xfffffff1;
>> >>     pci_busB->slot_reserved_mask = 0xfffffff0;
>> >> ------ snip -------
>> >> 
>> >> I think we could safely change the default value of
>> >> enforce_slot_reserved_mask_manual to false but set
>> >> it to true for the sparc64 sun4u board here to preserve
>> >> the current behavior of the only existing board in Qemu
>> >> that uses slot_reserved_mask.
>> >> 
>> >> What do you think?
>> > 
>> > I guess first can you answer whether this is still needed
>> > with the latest Xen patches?
>> > 
>> 
>> It's not really needed except for experimental purposes to allow
>> an administrator to test experimental configurations with a device
>> other than the igd at slot 2. That might be useful in some cases,
>> but it is not really necessary unless someone asks for that capability.
>> If libvirt users who ordinarily like to manually specify all the
>> settings will be OK with the proposed patch to xen that prevents
>> an administrator from being able to override a new setting that
>> reserves slot 2 for the igd for type "xenfv" machines configured for
>> igd passthrough, then there is no need for this patch. I don't think
>> many users need the capability to insert a different device in slot 2 for
>> the "xenfv" machine type configured with igd-passthru=on, so I would be
>> OK if this patch is not included in qemu.
>> 
>> Chuck
> 
> Pls wait and see if that patch gets picked up. Let me know.
> 

A day or two ago Anthony said he would look at the xen patch soon. So we'll
just wait for him, and I'll let you know if he is going to pull it up.

Re: [PATCH] pci: add enforce_slot_reserved_mask_manual property

[Chuck Zmudzinski]

On 1/28/23 2:14 PM, Michael S. Tsirkin wrote:
> On Sat, Jan 28, 2023 at 08:20:55AM -0500, Chuck Zmudzinski wrote:
> > On 1/28/23 5:26 AM, Michael S. Tsirkin wrote:
> > > On Fri, Jan 27, 2023 at 10:39:28PM -0500, Chuck Zmudzinski wrote:
> > >> On 1/27/2023 8:28 AM, Michael S. Tsirkin wrote:
> > >> > On Sun, Jan 15, 2023 at 07:49:51PM -0500, Chuck Zmudzinski wrote:
> > >> > > The current reserved slot check in do_pci_register_device(), added with
> > >> > > commit 8b8849844fd6
> > >> >
> > >> > add ("subject here") please
> > >> >
> > >> > > ,is done even if the pci device being added is
> > >> > > configured manually for a particular slot. The new property, when set
> > >> > > to false, disables the check when the device is configured to request a
> > >> > > particular slot. This allows an administrator or management tool to
> > >> > > override slot_reserved_mask for a pci device by requesting a particular
> > >> > > slot for the device. The new property is initialized to true which
> > >> > > preserves the existing behavior of slot_reserved_mask by default.
> > >> > > 
> > >> > > Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
> > >> >
> > >> > Thanks!
> > >> > I'm trying to think of the best default for this.
> > >> 
> > >> I think it would be better for the default value of
> > >> enforce_slot_reserved_mask_manual to be false, so that a
> > >> user-specified slot will by default override slot_reserved_mask.
> > >> But doing that would change the current behavior of
> > >> slot_reserved_mask.
> > >> 
> > >> Currently, this is the only place where slot_reserved_mask is used in all
> > >> of the Qemu source (code from hw/sparc64/sun4u.c):
> > >> 
> > >> ------ snip -------
> > >>     /* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is
> > >>        reserved (leaving no slots free after on-board devices) however slots
> > >>        0-3 are free on busB */
> > >>     pci_bus->slot_reserved_mask = 0xfffffffc;
> > >>     pci_busA->slot_reserved_mask = 0xfffffff1;
> > >>     pci_busB->slot_reserved_mask = 0xfffffff0;
> > >> ------ snip -------
> > >> 
> > >> I think we could safely change the default value of
> > >> enforce_slot_reserved_mask_manual to false but set
> > >> it to true for the sparc64 sun4u board here to preserve
> > >> the current behavior of the only existing board in Qemu
> > >> that uses slot_reserved_mask.
> > >> 
> > >> What do you think?
> > > 
> > > I guess first can you answer whether this is still needed
> > > with the latest Xen patches?
> > > 
> > 
> > It's not really needed except for experimental purposes to allow
> > an administrator to test experimental configurations with a device
> > other than the igd at slot 2. That might be useful in some cases,
> > but it is not really necessary unless someone asks for that capability.
> > If libvirt users who ordinarily like to manually specify all the
> > settings will be OK with the proposed patch to xen that prevents
> > an administrator from being able to override a new setting that
> > reserves slot 2 for the igd for type "xenfv" machines configured for
> > igd passthrough, then there is no need for this patch. I don't think
> > many users need the capability to insert a different device in slot 2 for
> > the "xenfv" machine type configured with igd-passthru=on, so I would be
> > OK if this patch is not included in qemu.
> > 
> > Chuck
>
> Pls wait and see if that patch gets picked up. Let me know.
>

Hi Anthony,

I didn't Cc you when I proposed a patch to core pci to answer Michael's
complaint that the patch Qemu to reserve slot 2 for the Intel IGD
unconditionally reserves slot 2 for the IGD for the "xenfv" machine when
the guest is configured with igd-passthru=on.

I probably should have Cc'd this patch to you even though you are not
a maintainer of core pci because the discussion of this patch to core pci is
relevant to your decision about which patch to use (if any) to improve
support for the intel IGD with Xen and upstream Qemu.

Michael, who is a maintainer of core pci, shows interest in this patch
if you decide to pull up the patch to Qemu that reserves slot 2 for the Intel
IGD when using the "xenfv" machine configured with igd-passthru=on.

As you see from this message, we are waiting on a decision of the
Xen maintainers about what patch, if any, will be applied to fix
the support for the Intel IGD on Xen in Qemu upstream.

I know you have quite a bit of information from me to process before you
make a decision about which patch to apply to fix the Intel IGD support in
upstream Qemu and Xen, so please take enough time to make the best
decision. If you need any more information from me, please let me know
by replying to one of my proposed patches to fix support for the Intel IGD.

If you are subscribed to qemu-devel, you should have the full discussion of
this patch to Qemu core pci in your inbox. You can also access the proposed
patch and the discussion of the patch here:

https://lore.kernel.org/qemu-devel/ad5f5cf8bc4bd4a720724ed41e47565a7f27adf5.1673829387.git.brchuckz@aol.com/

Kind regards,

Chuck

Re: [PATCH] pci: add enforce_slot_reserved_mask_manual property

[Mark Cave-Ayland]

On 28/01/2023 03:39, Chuck Zmudzinski wrote:

> On 1/27/2023 8:28 AM, Michael S. Tsirkin wrote:
>> On Sun, Jan 15, 2023 at 07:49:51PM -0500, Chuck Zmudzinski wrote:
>>> The current reserved slot check in do_pci_register_device(), added with
>>> commit 8b8849844fd6
>>
>> add ("subject here") please
>>
>>> ,is done even if the pci device being added is
>>> configured manually for a particular slot. The new property, when set
>>> to false, disables the check when the device is configured to request a
>>> particular slot. This allows an administrator or management tool to
>>> override slot_reserved_mask for a pci device by requesting a particular
>>> slot for the device. The new property is initialized to true which
>>> preserves the existing behavior of slot_reserved_mask by default.
>>>
>>> Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
>>
>> Thanks!
>> I'm trying to think of the best default for this.
> 
> I think it would be better for the default value of
> enforce_slot_reserved_mask_manual to be false, so that a
> user-specified slot will by default override slot_reserved_mask.
> But doing that would change the current behavior of
> slot_reserved_mask.
> 
> Currently, this is the only place where slot_reserved_mask is used in all
> of the Qemu source (code from hw/sparc64/sun4u.c):
> 
> ------ snip -------
>      /* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is
>         reserved (leaving no slots free after on-board devices) however slots
>         0-3 are free on busB */
>      pci_bus->slot_reserved_mask = 0xfffffffc;
>      pci_busA->slot_reserved_mask = 0xfffffff1;
>      pci_busB->slot_reserved_mask = 0xfffffff0;
> ------ snip -------
> 
> I think we could safely change the default value of
> enforce_slot_reserved_mask_manual to false but set
> it to true for the sparc64 sun4u board here to preserve
> the current behavior of the only existing board in Qemu
> that uses slot_reserved_mask.
> 
> What do you think?
> 
>> Users is trying to configure a specific device on a reserved
>> slot. Should we
>> CC a bunch more people for visibility. Input, anyone?

For a bit of background, slot_reserved_mask was added by me to solve a problem with 
the sun4u machine: on a real Ultra-5, the pci "A" bus has 2 free slots and the pci 
"B" bus has 4 free slots. Whilst it is possible to plug a PCI device into any slot in 
QEMU, the PCI bridges only have IRQ mapping registers for those 6 slots, so you can 
easily end up with an auto-allocated slot where it is impossible for the OS to map 
the IRQ.

Hence slot_reserved_mask was originally intended to mark slots as being unavailable 
for both manual and automatic allocation to ensure that devices plugged into both PCI 
buses would always work.

If there is a need to change/refactor the logic then I can test the sun4u machine to 
ensure the original test case still works.


ATB,

Mark.

Re: [PATCH] pci: add enforce_slot_reserved_mask_manual property

[Chuck Zmudzinski]

On 1/28/2023 4:58 PM, Mark Cave-Ayland wrote:
> On 28/01/2023 03:39, Chuck Zmudzinski wrote:
>
> > On 1/27/2023 8:28 AM, Michael S. Tsirkin wrote:
> >> On Sun, Jan 15, 2023 at 07:49:51PM -0500, Chuck Zmudzinski wrote:
> >>> The current reserved slot check in do_pci_register_device(), added with
> >>> commit 8b8849844fd6
> >>
> >> add ("subject here") please
> >>
> >>> ,is done even if the pci device being added is
> >>> configured manually for a particular slot. The new property, when set
> >>> to false, disables the check when the device is configured to request a
> >>> particular slot. This allows an administrator or management tool to
> >>> override slot_reserved_mask for a pci device by requesting a particular
> >>> slot for the device. The new property is initialized to true which
> >>> preserves the existing behavior of slot_reserved_mask by default.
> >>>
> >>> Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
> >>
> >> Thanks!
> >> I'm trying to think of the best default for this.
> > 
> > I think it would be better for the default value of
> > enforce_slot_reserved_mask_manual to be false, so that a
> > user-specified slot will by default override slot_reserved_mask.
> > But doing that would change the current behavior of
> > slot_reserved_mask.
> > 
> > Currently, this is the only place where slot_reserved_mask is used in all
> > of the Qemu source (code from hw/sparc64/sun4u.c):
> > 
> > ------ snip -------
> >      /* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is
> >         reserved (leaving no slots free after on-board devices) however slots
> >         0-3 are free on busB */
> >      pci_bus->slot_reserved_mask = 0xfffffffc;
> >      pci_busA->slot_reserved_mask = 0xfffffff1;
> >      pci_busB->slot_reserved_mask = 0xfffffff0;
> > ------ snip -------
> > 
> > I think we could safely change the default value of
> > enforce_slot_reserved_mask_manual to false but set
> > it to true for the sparc64 sun4u board here to preserve
> > the current behavior of the only existing board in Qemu
> > that uses slot_reserved_mask.
> > 
> > What do you think?
> > 
> >> Users is trying to configure a specific device on a reserved
> >> slot. Should we
> >> CC a bunch more people for visibility. Input, anyone?
>
> For a bit of background, slot_reserved_mask was added by me to solve a problem with 
> the sun4u machine: on a real Ultra-5, the pci "A" bus has 2 free slots and the pci 
> "B" bus has 4 free slots. Whilst it is possible to plug a PCI device into any slot in 
> QEMU, the PCI bridges only have IRQ mapping registers for those 6 slots, so you can 
> easily end up with an auto-allocated slot where it is impossible for the OS to map 
> the IRQ.
>
> Hence slot_reserved_mask was originally intended to mark slots as being unavailable 
> for both manual and automatic allocation to ensure that devices plugged into both PCI 
> buses would always work.
>
> If there is a need to change/refactor the logic then I can test the sun4u machine to 
> ensure the original test case still works.
>
>
> ATB,
>
> Mark.

Thanks, I will let you know if there is a patch to test on the
sun4u machine. For now, we are waiting to see if the xen
maintainers will accept a patch that uses slot_reserved_mask
to prevent other devices from using the slot that is required
by the Intel igd in the xenfv machine. That patch does not change
the way slot_reserved_mask works, but if that patch is added
some users might want to add a capability for a user to override
slot_reserved_mask, and that is what this patch attempts to
implement.

Kind regards,

Chuck

Re: [PATCH] pci: add enforce_slot_reserved_mask_manual property

[Chuck Zmudzinski]

On 1/28/2023 4:58 PM, Mark Cave-Ayland wrote:
> On 28/01/2023 03:39, Chuck Zmudzinski wrote:
>
> > On 1/27/2023 8:28 AM, Michael S. Tsirkin wrote:
> >> On Sun, Jan 15, 2023 at 07:49:51PM -0500, Chuck Zmudzinski wrote:
> >>> The current reserved slot check in do_pci_register_device(), added with
> >>> commit 8b8849844fd6
> >>
> >> add ("subject here") please
> >>
> >>> ,is done even if the pci device being added is
> >>> configured manually for a particular slot. The new property, when set
> >>> to false, disables the check when the device is configured to request a
> >>> particular slot. This allows an administrator or management tool to
> >>> override slot_reserved_mask for a pci device by requesting a particular
> >>> slot for the device. The new property is initialized to true which
> >>> preserves the existing behavior of slot_reserved_mask by default.
> >>>
> >>> Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
> >>
> >> Thanks!
> >> I'm trying to think of the best default for this.

Hi Michael and Mark,

The Xen maintainers pulled up the commit that reserves slot 2
for the Intel IGD with the xenfv machine so we need to discuss
how slot_reserved_mask should work with this change. The original
version of the patch keeps the current default of always enforcing
slot_reserved_mask, even if the administrator tries to configure the
slots manually. So if we keep that behavior, we will need to patch
the xenfv machine again to implement the desired behavior that
slot_reserved_mask will only apply with automatic slot assignment
for the xenfv machine. That would be a fairly trivial one-line patch
to add on top of the patch the Xen maintainers just pulled up.

Another option is to change the default value of
enforce_slot_reserved_mask_manual to false so slot_reserved_mask
only affects automatic slot address assignment by default and then
preserve the current behavior for the sun4u machine by patching the
sun4u machine so the value of enforce_slot_reserved_mask_manual
is true for the sun4u machine.

> > 
> > I think it would be better for the default value of
> > enforce_slot_reserved_mask_manual to be false, so that a
> > user-specified slot will by default override slot_reserved_mask.
> > But doing that would change the current behavior of
> > slot_reserved_mask.
> > 
> > Currently, this is the only place where slot_reserved_mask is used in all
> > of the Qemu source (code from hw/sparc64/sun4u.c):
> > 
> > ------ snip -------
> >      /* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is
> >         reserved (leaving no slots free after on-board devices) however slots
> >         0-3 are free on busB */
> >      pci_bus->slot_reserved_mask = 0xfffffffc;
> >      pci_busA->slot_reserved_mask = 0xfffffff1;
> >      pci_busB->slot_reserved_mask = 0xfffffff0;
> > ------ snip -------
> > 
> > I think we could safely change the default value of
> > enforce_slot_reserved_mask_manual to false but set
> > it to true for the sparc64 sun4u board here to preserve
> > the current behavior of the only existing board in Qemu
> > that uses slot_reserved_mask.
> > 
> > What do you think?
> > 
> >> Users is trying to configure a specific device on a reserved
> >> slot. Should we
> >> CC a bunch more people for visibility. Input, anyone?
>
> For a bit of background, slot_reserved_mask was added by me to solve a problem with 
> the sun4u machine: on a real Ultra-5, the pci "A" bus has 2 free slots and the pci 
> "B" bus has 4 free slots. Whilst it is possible to plug a PCI device into any slot in 
> QEMU, the PCI bridges only have IRQ mapping registers for those 6 slots, so you can 
> easily end up with an auto-allocated slot where it is impossible for the OS to map 
> the IRQ.
>
> Hence slot_reserved_mask was originally intended to mark slots as being unavailable 
> for both manual and automatic allocation to ensure that devices plugged into both PCI 
> buses would always work.

Here is a third option:

Mark, would it be acceptable to replace the error in the case of manual
allocation with a warning, so manual device assignment to a reserved slot
in the sun4u machine would succeed (automatic assignment would still
prevent the slot from being used) but there would be a warning message
to provide the administrator/user with a clue that the current manual
configuration of the device is not correct and could cause problems?

That would be a less aggressive change that could be implemented without
needing to add the enforce_slot_reserved_mask_manual property to PCIBus.

Also, Michael, I notice that the current usage of slot_reserved_mask violates
the comment at the beginning of pci_bus.h that asks that the properties of
PCIBus such as slot_reserved_mask be accessed via accessor functions in pci.h
instead of directly accessing them. Should I also write v2 of the patch to add
accessor functions for slot_reserved_mask so the accessor functions can be used
instead of accessing slot_reserved_mask directly?

I will wait a little while for some feedback before posting v2 of this patch.

> If there is a need to change/refactor the logic then I can test the sun4u machine to 
> ensure the original test case still works.

Sounds good... Best regards,

Chuck

> ATB,
>
> Mark.

Re: [PATCH] pci: add enforce_slot_reserved_mask_manual property

[Mark Cave-Ayland]

On 06/03/2023 16:37, Chuck Zmudzinski wrote:

> On 1/28/2023 4:58 PM, Mark Cave-Ayland wrote:
>> On 28/01/2023 03:39, Chuck Zmudzinski wrote:
>>
>>> On 1/27/2023 8:28 AM, Michael S. Tsirkin wrote:
>>>> On Sun, Jan 15, 2023 at 07:49:51PM -0500, Chuck Zmudzinski wrote:
>>>>> The current reserved slot check in do_pci_register_device(), added with
>>>>> commit 8b8849844fd6
>>>>
>>>> add ("subject here") please
>>>>
>>>>> ,is done even if the pci device being added is
>>>>> configured manually for a particular slot. The new property, when set
>>>>> to false, disables the check when the device is configured to request a
>>>>> particular slot. This allows an administrator or management tool to
>>>>> override slot_reserved_mask for a pci device by requesting a particular
>>>>> slot for the device. The new property is initialized to true which
>>>>> preserves the existing behavior of slot_reserved_mask by default.
>>>>>
>>>>> Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
>>>>
>>>> Thanks!
>>>> I'm trying to think of the best default for this.
> 
> Hi Michael and Mark,
> 
> The Xen maintainers pulled up the commit that reserves slot 2
> for the Intel IGD with the xenfv machine so we need to discuss
> how slot_reserved_mask should work with this change. The original
> version of the patch keeps the current default of always enforcing
> slot_reserved_mask, even if the administrator tries to configure the
> slots manually. So if we keep that behavior, we will need to patch
> the xenfv machine again to implement the desired behavior that
> slot_reserved_mask will only apply with automatic slot assignment
> for the xenfv machine. That would be a fairly trivial one-line patch
> to add on top of the patch the Xen maintainers just pulled up.
> 
> Another option is to change the default value of
> enforce_slot_reserved_mask_manual to false so slot_reserved_mask
> only affects automatic slot address assignment by default and then
> preserve the current behavior for the sun4u machine by patching the
> sun4u machine so the value of enforce_slot_reserved_mask_manual
> is true for the sun4u machine.
> 
>>>
>>> I think it would be better for the default value of
>>> enforce_slot_reserved_mask_manual to be false, so that a
>>> user-specified slot will by default override slot_reserved_mask.
>>> But doing that would change the current behavior of
>>> slot_reserved_mask.
>>>
>>> Currently, this is the only place where slot_reserved_mask is used in all
>>> of the Qemu source (code from hw/sparc64/sun4u.c):
>>>
>>> ------ snip -------
>>>       /* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is
>>>          reserved (leaving no slots free after on-board devices) however slots
>>>          0-3 are free on busB */
>>>       pci_bus->slot_reserved_mask = 0xfffffffc;
>>>       pci_busA->slot_reserved_mask = 0xfffffff1;
>>>       pci_busB->slot_reserved_mask = 0xfffffff0;
>>> ------ snip -------
>>>
>>> I think we could safely change the default value of
>>> enforce_slot_reserved_mask_manual to false but set
>>> it to true for the sparc64 sun4u board here to preserve
>>> the current behavior of the only existing board in Qemu
>>> that uses slot_reserved_mask.
>>>
>>> What do you think?
>>>
>>>> Users is trying to configure a specific device on a reserved
>>>> slot. Should we
>>>> CC a bunch more people for visibility. Input, anyone?
>>
>> For a bit of background, slot_reserved_mask was added by me to solve a problem with
>> the sun4u machine: on a real Ultra-5, the pci "A" bus has 2 free slots and the pci
>> "B" bus has 4 free slots. Whilst it is possible to plug a PCI device into any slot in
>> QEMU, the PCI bridges only have IRQ mapping registers for those 6 slots, so you can
>> easily end up with an auto-allocated slot where it is impossible for the OS to map
>> the IRQ.
>>
>> Hence slot_reserved_mask was originally intended to mark slots as being unavailable
>> for both manual and automatic allocation to ensure that devices plugged into both PCI
>> buses would always work.
> 
> Here is a third option:
> 
> Mark, would it be acceptable to replace the error in the case of manual
> allocation with a warning, so manual device assignment to a reserved slot
> in the sun4u machine would succeed (automatic assignment would still
> prevent the slot from being used) but there would be a warning message
> to provide the administrator/user with a clue that the current manual
> configuration of the device is not correct and could cause problems?
> 
> That would be a less aggressive change that could be implemented without
> needing to add the enforce_slot_reserved_mask_manual property to PCIBus.

Unfortunately in the sun4u case that doesn't quite work, since the PCI host bridge 
doesn't have IRQ routing registers for the reserved slots and so the card would still 
fail with a manual allocation.

In effect the reserved bit in its current implementation means "this slot is 
unavailable" which is the intended result for the original implementation. Certainly 
this logic would reduce the changes of creating an unusable setup from the command 
line, but then some management tools which manually allocates slots would still allow 
an unusable configuration.

> Also, Michael, I notice that the current usage of slot_reserved_mask violates
> the comment at the beginning of pci_bus.h that asks that the properties of
> PCIBus such as slot_reserved_mask be accessed via accessor functions in pci.h
> instead of directly accessing them. Should I also write v2 of the patch to add
> accessor functions for slot_reserved_mask so the accessor functions can be used
> instead of accessing slot_reserved_mask directly?
> 
> I will wait a little while for some feedback before posting v2 of this patch.

Another possibility could be to move the slot validation logic in 
do_pci_register_device() to a separate function, and add a new slot validation 
callback to PCIBusClass to be used by do_pci_register_device() instead. By default 
this would call the existing slot validation logic function.

It would then be possible to override the default slot validation function in the Xen 
case, perhaps even calling the existing logic first before adding your additional 
logic requirement on top.


ATB,

Mark.

Re: [PATCH] pci: add enforce_slot_reserved_mask_manual property

[Chuck Zmudzinski]

On 3/12/2023 10:13 AM, Mark Cave-Ayland wrote:
> On 06/03/2023 16:37, Chuck Zmudzinski wrote:
>
> > On 1/28/2023 4:58 PM, Mark Cave-Ayland wrote:
> >> On 28/01/2023 03:39, Chuck Zmudzinski wrote:
> >>
> >>> On 1/27/2023 8:28 AM, Michael S. Tsirkin wrote:
> >>>> On Sun, Jan 15, 2023 at 07:49:51PM -0500, Chuck Zmudzinski wrote:
> >>>>> The current reserved slot check in do_pci_register_device(), added with
> >>>>> commit 8b8849844fd6
> >>>>
> >>>> add ("subject here") please
> >>>>
> >>>>> ,is done even if the pci device being added is
> >>>>> configured manually for a particular slot. The new property, when set
> >>>>> to false, disables the check when the device is configured to request a
> >>>>> particular slot. This allows an administrator or management tool to
> >>>>> override slot_reserved_mask for a pci device by requesting a particular
> >>>>> slot for the device. The new property is initialized to true which
> >>>>> preserves the existing behavior of slot_reserved_mask by default.
> >>>>>
> >>>>> Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
> >>>>
> >>>> Thanks!
> >>>> I'm trying to think of the best default for this.
> > 
> > Hi Michael and Mark,
> > 
> > The Xen maintainers pulled up the commit that reserves slot 2
> > for the Intel IGD with the xenfv machine so we need to discuss
> > how slot_reserved_mask should work with this change. The original
> > version of the patch keeps the current default of always enforcing
> > slot_reserved_mask, even if the administrator tries to configure the
> > slots manually. So if we keep that behavior, we will need to patch
> > the xenfv machine again to implement the desired behavior that
> > slot_reserved_mask will only apply with automatic slot assignment
> > for the xenfv machine. That would be a fairly trivial one-line patch
> > to add on top of the patch the Xen maintainers just pulled up.
> > 
> > Another option is to change the default value of
> > enforce_slot_reserved_mask_manual to false so slot_reserved_mask
> > only affects automatic slot address assignment by default and then
> > preserve the current behavior for the sun4u machine by patching the
> > sun4u machine so the value of enforce_slot_reserved_mask_manual
> > is true for the sun4u machine.
> > 
> >>>
> >>> I think it would be better for the default value of
> >>> enforce_slot_reserved_mask_manual to be false, so that a
> >>> user-specified slot will by default override slot_reserved_mask.
> >>> But doing that would change the current behavior of
> >>> slot_reserved_mask.
> >>>
> >>> Currently, this is the only place where slot_reserved_mask is used in all
> >>> of the Qemu source (code from hw/sparc64/sun4u.c):
> >>>
> >>> ------ snip -------
> >>>       /* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is
> >>>          reserved (leaving no slots free after on-board devices) however slots
> >>>          0-3 are free on busB */
> >>>       pci_bus->slot_reserved_mask = 0xfffffffc;
> >>>       pci_busA->slot_reserved_mask = 0xfffffff1;
> >>>       pci_busB->slot_reserved_mask = 0xfffffff0;
> >>> ------ snip -------
> >>>
> >>> I think we could safely change the default value of
> >>> enforce_slot_reserved_mask_manual to false but set
> >>> it to true for the sparc64 sun4u board here to preserve
> >>> the current behavior of the only existing board in Qemu
> >>> that uses slot_reserved_mask.
> >>>
> >>> What do you think?
> >>>
> >>>> Users is trying to configure a specific device on a reserved
> >>>> slot. Should we
> >>>> CC a bunch more people for visibility. Input, anyone?
> >>
> >> For a bit of background, slot_reserved_mask was added by me to solve a problem with
> >> the sun4u machine: on a real Ultra-5, the pci "A" bus has 2 free slots and the pci
> >> "B" bus has 4 free slots. Whilst it is possible to plug a PCI device into any slot in
> >> QEMU, the PCI bridges only have IRQ mapping registers for those 6 slots, so you can
> >> easily end up with an auto-allocated slot where it is impossible for the OS to map
> >> the IRQ.
> >>
> >> Hence slot_reserved_mask was originally intended to mark slots as being unavailable
> >> for both manual and automatic allocation to ensure that devices plugged into both PCI
> >> buses would always work.
> > 
> > Here is a third option:
> > 
> > Mark, would it be acceptable to replace the error in the case of manual
> > allocation with a warning, so manual device assignment to a reserved slot
> > in the sun4u machine would succeed (automatic assignment would still
> > prevent the slot from being used) but there would be a warning message
> > to provide the administrator/user with a clue that the current manual
> > configuration of the device is not correct and could cause problems?
> > 
> > That would be a less aggressive change that could be implemented without
> > needing to add the enforce_slot_reserved_mask_manual property to PCIBus.
>
> Unfortunately in the sun4u case that doesn't quite work, since the PCI host bridge 
> doesn't have IRQ routing registers for the reserved slots and so the card would still 
> fail with a manual allocation.
>
> In effect the reserved bit in its current implementation means "this slot is 
> unavailable" which is the intended result for the original implementation. Certainly 
> this logic would reduce the changes of creating an unusable setup from the command 
> line, but then some management tools which manually allocates slots would still allow 
> an unusable configuration.

OK, I will not change the current behavior for the sun4u machine.

>
> > Also, Michael, I notice that the current usage of slot_reserved_mask violates
> > the comment at the beginning of pci_bus.h that asks that the properties of
> > PCIBus such as slot_reserved_mask be accessed via accessor functions in pci.h
> > instead of directly accessing them. Should I also write v2 of the patch to add
> > accessor functions for slot_reserved_mask so the accessor functions can be used
> > instead of accessing slot_reserved_mask directly?
> > 
> > I will wait a little while for some feedback before posting v2 of this patch.
>
> Another possibility could be to move the slot validation logic in 
> do_pci_register_device() to a separate function, and add a new slot validation 
> callback to PCIBusClass to be used by do_pci_register_device() instead. By default 
> this would call the existing slot validation logic function.
>
> It would then be possible to override the default slot validation function in the Xen 
> case, perhaps even calling the existing logic first before adding your additional 
> logic requirement on top.
I agree we can keep the default to be the logic that the sun4u machine currently
uses and override it to achieve the desired behavior of the xenfv machine.

Kind regards,

Chuck

>
>
> ATB,
>
> Mark.