Re: [RFC PATCH 1/1] virtio: fix feature negotiation for ACCESS_PLATFORM

[Halil Pasic]

On Mon, 7 Feb 2022 16:46:04 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> On 2/7/22 11:46, Halil Pasic wrote:
> > On Mon, 7 Feb 2022 08:46:34 -0300
> > Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
> >   
> > I have considered this and decided against it. The reason why is
> > if that approach is taken, we can't really add more code to the
> > end of the function. An early return is good if we want to
> > abort the function with an error. My point is !has_iommu does
> > not necessarily mean we are done: after a block that handles
> > the has_iommu situation, in future, there could be a block that
> > handles something different.  
> 
> And that's fine, but the way this patch is changing it I'm not sure it's 
> better
> than what we already have. Today we have:
> 
> if (has_iommu) {

To be exact today we have :
if (klass->get_dma_as != NULL && has_iommu) {


>    (... assign vdev->dma_as in some cases ...)

Today not in some case but unconditionally. WE already checked for
!!klass->get_dma_as and that is important.

Because if you rewrite current to what you have just described here,
then in this branch of the if-else you have to handle !klass->get_dma_as.

So you would have to do
    if (klass->get_dma_as) {
        vdev->dma_as = klass->get_dma_as();
        if (cond) {
            do_error();
        }
    } else {
        vdev->dma_as = &address_space_memory;
    }

> } else {
>     vdev->dma_as = &address_space_memory;
> }
> 
> 
> Your patch is doing:
> 
> vdev->dma_as = &address_space_memory;
> 
> if (has_iommu) {
>    (... assign vdev->dma_as in some cases ...)
> }
> 
> 
> You got rid of an 'else', but ended up adding a double "vdev->dma_as =" 
> assignment
> depending on the case (has_iommu = true and klass->get_dma_as != NULL). 

And why is that bad?

The solution I wrote is very clear about vdev->dma_as != NULL and that
vdev->dma_as conceptually defaults to &address_space_memory, and may
deviate from that only if both has_iommu and klass->get_dma_as != NULL
in which case get_dma_as() may override it to something different.

The compile can still generate branches and stores as it pleases
as long as the behavior is the same AFAIK. 

> This is why
> I proposed the early exit.
> 
> If we're worried about adding more code in the future might as well leave the 
> existing
> if/else as is.
> 

Not really, we would end up having two extra else branches instead of
none (with 3 if-s in both cases) and 3 places where we might assign
->dma_as (although mutually exclusive) instead of just two.

For me my version is easier to read.

        
> 
> 
> > 
> > Would this patch work for power? Or are there valid scenarios that
> > it breaks? I'm asking, because you voiced concern regarding this before.  
> 
> 
> I'll test it when I have an opportunity and let you know.
> 
> 

Thank you!

Regards,
Halil