Re: [PATCH v20 2/2] vpci: translate virtual PCI bus topology for guests

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 5/7/25 03:44, Roger Pau Monné wrote:
> On Tue, May 06, 2025 at 11:05:13PM -0400, Stewart Hildebrand wrote:
>> On 5/6/25 07:16, Roger Pau Monné wrote:
>>> On Fri, Apr 18, 2025 at 02:58:37PM -0400, Stewart Hildebrand wrote:
>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>>>  static int vpci_register_cmp(const struct vpci_register *r1,
>>>>                               const struct vpci_register *r2)
>>>>  {
>>>> @@ -438,7 +473,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, 
>>>> unsigned int size)
>>>>      const struct pci_dev *pdev;
>>>>      const struct vpci_register *r;
>>>>      unsigned int data_offset = 0;
>>>> -    uint32_t data = ~(uint32_t)0;
>>>> +    uint32_t data = 0xffffffffU >> (32 - 8 * size);
>>>
>>> This seems kind of unrelated to the rest of the code in the patch,
>>> why is this needed?  Isn't it always fine to return all ones, and let
>>> the caller truncate to the required size?
>>>
>>> Otherwise the code in vpci_read_hw() also needs to be adjusted.
>>
>> On Arm, since 9a5e22b64266 ("xen/arm: check read handler behavior") we
>> assert that the read handlers don't set any bits above the access size.
> 
> I see.  That kind of diverges from x86 behavior, that AFAICT (see
> memcpy() at tail of hvmemul_do_io()) instead truncates the memcpy to
> the size of the access.
> 
> Maybe it would be better to instead of asserting just truncate the
> returned value to the given size, as that would allow to just return
> ~0 from handlers without having to care about the specific access
> size.

The impression I get from [0] is that that on Arm, there's no benefit to
performing truncation in xen/arch/arm/io.c. Doing so would needlessly
affect other Arm internal read handlers (e.g. vGIC). For vPCI
specifically, however, we could potentially perform truncation in
xen/arch/arm/vpci.c. So I guess it's a question of whether we want to
give special treatment to vPCI compared to all other read handlers on
Arm?

>> I had adjusted data here due to returning it directly from vpci_read()
>> in the current form of the patch. With your suggestion below we would
>> only need to adjust vpci_read_hw() (and then data here would not
>> strictly need adjusting).
> 
> Both returns would need adjusting IMO,

OK

> and it should have been part of
> 9a5e22b64266 I think, since that's the commit that introduced the
> checking.

If we proceed with adjusting vpci_read() and vpci_read_hw(): are you OK
with these adjustments included in this patch, or would you prefer them
being split into a pre-patch?

[0] 
https://lore.kernel.org/xen-devel/20240522225927.77398-1-stewart.hildebrand@xxxxxxx/T/#t