Re: [PATCH v7 1/2] xen/vpci: header: status register handler

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Nov 23, 2023 at 07:57:21AM -0500, Stewart Hildebrand wrote:
> On 11/23/23 03:14, Roger Pau Monné wrote:
> > On Wed, Nov 22, 2023 at 03:16:29PM -0500, Stewart Hildebrand wrote:
> >> On 11/17/23 07:40, Roger Pau Monné wrote:
> >>> On Wed, Sep 13, 2023 at 10:35:46AM -0400, Stewart Hildebrand wrote:
> >>>>      r->write(pdev, r->offset, data & (0xffffffffU >> (32 - 8 * 
> >>>> r->size)),
> >>>>               r->private);
> >>>>  }
> >>>> diff --git a/xen/include/xen/pci_regs.h b/xen/include/xen/pci_regs.h
> >>>> index 84b18736a85d..b72131729db6 100644
> >>>> --- a/xen/include/xen/pci_regs.h
> >>>> +++ b/xen/include/xen/pci_regs.h
> >>>> @@ -66,6 +66,15 @@
> >>>>  #define  PCI_STATUS_REC_MASTER_ABORT    0x2000 /* Set on master abort */
> >>>>  #define  PCI_STATUS_SIG_SYSTEM_ERROR    0x4000 /* Set when we drive 
> >>>> SERR */
> >>>>  #define  PCI_STATUS_DETECTED_PARITY     0x8000 /* Set on parity error */
> >>>> +#define  PCI_STATUS_RSVDZ_MASK          0x0006
> >>>
> >>> In my copy of the PCIe 6 spec bit 6 is also RsvdZ, so the mask should
> >>> be 0x46.
> >>
> >> Right, mine too. It's probably safer to follow the newer version of the 
> >> spec, so I'll make the change. For completeness / archaeology purposes, I 
> >> just want to document some relevant data points here.
> >>
> >> In PCIe 4 spec, it says this about bit 6:
> >> "These bits were used in previous versions of the programming model. 
> >> Careful consideration should be given to any attempt to repurpose them."
> >>
> >> Going further back, PCI (old school PCI, not Express) spec 3.0 says this 
> >> about bit 6:
> >> "This bit is reserved. *"
> >> "* In Revision 2.1 of this specification, this bit was used to indicate 
> >> whether or not a device supported User Definable Features."
> >>
> >> Just above in our pci_regs.h (and equally in Linux 
> >> include/uapi/linux/pci_regs.h) we have this definition for bit 6:
> >> #define  PCI_STATUS_UDF         0x40    /* Support User Definable Features 
> >> [obsolete] */
> >>
> >> Qemu hw/xen/xen_pt_config_init.c treats bit 6 as RO:
> >>         .ro_mask    = 0x06F8,
> > 
> > Right, given the implementation of ro_mask that would likely be fine.
> > Reading unconditionally as 0 while preserving the value on writes
> > seems the safest option.
> 
> That would mean treating bit 6 as RsvdP, even though the PCIe 6 spec
> says RsvdZ. I just want to confirm this is indeed the intent since
> we both said RsvdZ just a moment ago? If so, I would add a comment
> since it's deviating from spec. I would personally still vote in
> favor of following PCIe 6 spec (RsvdZ).

Hm, yes, lets go with the spec and use RsdvZ.  I very much doubt we
passthrough any device that actually uses the UDF bit.

Thanks, Roger.