On Mon, 7 Feb 2022 16:46:04 -0300
Daniel Henrique Barboza <danielhb...@gmail.com> wrote:
> On 2/7/22 11:46, Halil Pasic wrote:
> > On Mon, 7 Feb 2022 08:46:34 -0300
> > Daniel Henrique Barboza <danielhb...@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
