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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 5/7/25 13:44, Roger Pau Monné wrote:
> On Wed, May 07, 2025 at 09:38:51AM -0400, Stewart Hildebrand wrote:
>> 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).
> 
> But isn't this truncation desirable in order to avoid possibly setting
> bits outside of the access size?

On Arm we expect the read handlers to have the bits above the access
size zeroed. If a read handler sets bits above the access size, it could
indicate a bug in the read handler. As a reminder, this was already
discussed at [0] and a patch was already committed 9a5e22b64266
("xen/arm: check read handler behavior"). Perhaps we could both keep the
ASSERT (for debug builds) and perform truncation (for release builds) in
xen/arch/arm/io.c:handle_read(), but that's patch for another day.

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

>> 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 would think doing the truncation uniformly for all reads would be
> better, as we then ensure the value propagated to the registers always
> matches the access size?
> 
> I'm not expert on ARM, but it seems cumbersome to force this to all
> internal handlers, instead of just truncating the value in a single
> place.

To move this forward, I suggest performing this truncation in
xen/arch/arm/vpci.c:vpci_mmio_read(). This will be a single place to
perform truncation for Arm vPCI, and will not affect other Arm internal
mmio handlers.