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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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).