On 11/21/23 11:27, Stewart Hildebrand wrote:
> On 11/21/23 10:18, Roger Pau Monné wrote:
>> On Tue, Nov 21, 2023 at 10:03:01AM -0500, Stewart Hildebrand wrote:
>>> On 11/21/23 09:45, Roger Pau Monné wrote:
>>>> On Wed, Sep 13, 2023 at 10:35:46AM -0400, Stewart Hildebrand wrote:
>>>>> @@ -407,26 +439,25 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int
>>>>> reg, unsigned int size)
>>>>>
>>>>> /*
>>>>> * Perform a maybe partial write to a register.
>>>>> - *
>>>>> - * Note that this will only work for simple registers, if Xen needs to
>>>>> - * trap accesses to rw1c registers (like the status PCI header register)
>>>>> - * the logic in vpci_write will have to be expanded in order to correctly
>>>>> - * deal with them.
>>>>> */
>>>>> static void vpci_write_helper(const struct pci_dev *pdev,
>>>>> const struct vpci_register *r, unsigned
>>>>> int size,
>>>>> unsigned int offset, uint32_t data)
>>>>> {
>>>>> + uint32_t val = 0;
>>>>> +
>>>>> ASSERT(size <= r->size);
>>>>>
>>>>> - if ( size != r->size )
>>>>> + if ( (size != r->size) || r->ro_mask )
>>>>> {
>>>>> - uint32_t val;
>>>>> -
>>>>> val = r->read(pdev, r->offset, r->private);
>>>>> + val &= ~r->rw1c_mask;
>>>>> data = merge_result(val, data, size, offset);
>>>>> }
>>>>>
>>>>> + data &= ~(r->rsvdz_mask | r->ro_mask);
>>>>> + data |= val & r->ro_mask;
>>>>
>>>> I've been thinking about this, and the way the ro_mask is implemented
>>>> (and the way we want to handle ro bits) is the same behavior as RsvdP.
>>>> I would suggest to rename the ro_mask to rsvdp_mask and note
>>>> that for resilience reasons we will handle RO bits as RsvdP.
>>>
>>> But the reads behave differently. RO should return the value, RsvdP should
>>> return 0 when read (according to the PCIe Base Spec 4.0).
>>
>> Hm, right, sorry for the wrong suggestion. We should force bits to 0
>> for guests reads, but make sure that for writes the value on the
>> hardware is preserved.
>>
>> So we need the separate mask for RsvdP, which will be handled like
>> ro_mask in vpci_write_helper() and like rsvdz_mask in vpci_read().
>
> Agreed. The only reason I didn't add RsvdP support in this patch was that it
> wasn't needed for the status register... Since RsvdP will be needed for the
> command register, I think I may as well implement it as part of this series,
> perhaps in a separate patch following this one. Stay tuned for v8.
I just remembered that RsvdP would actually be useful for the
PCI_STATUS_CAP_LIST bit in the status register. So I'll implement it directly
in this patch afterall, not a separate one.
