Re: [RFC 07/55] KVM: arm/arm64: Add virtual EL2 state emulation framework

[Bandan Das <bsd@redhat.com>]

Christoffer Dall <cdall@linaro.org> writes:

> [off list]

Please keep discussions on list.
...
> No, it's actually not.  You can have entries, which are the same, and
> entire tables where the guest's page table happen to be the same as what
> gets put in the shadow page table.  I've worked on hypervisors where
> this was always pretty much the case, because memory was segmented and
> physical addresses were exposed to the guest.

Ok cool, I believe you!

> I don't need a lecture from you about how shadow page tables work.
>

Go back to the review I posted. I merely asked if using "shadow" in the name could
be confusing and if you think it isn't, you could have just pointed it out and that
would have been the end of it.

> Nevertheless, I was just trying to explain our rationale for choosing
> the word shadow in our naming, and I suggest in the future you make
> constructive comments with suggesting a better name, instead of pointing
> out your dissatiscation, which is not helpful.
>
>> >> When it's pointing to EL1 state, it's not really
>> >> shadow state anymore.
>> >> 
>> >
>> > You can argue it both ways, in the end, all that's important is whether
>> > or not it's clear what the functions do.
>> >
>> >> > If you have better suggestions for naming, we're open to that though.
>> >> >
>> >> 
>> >> Oh nothing specifically, I just felt like "shadow" in the function name
>> >> could be confusing. Borrowing from kvm_arm_init_cpu_context(), 
>> >> how about kvm_arm_setup/restore_cpu_context()  ?
>> >
>> > I have no objection to these names.
>> >
>> >> 
>> >> BTW, on a separate note, we might as well get away with the typedef and
>> >> call struct kvm_cpu_context directly.
>> >> 
>> > I don't think it's worth changing the code just for that, but if you
>> > feel it's a significant cleanup, you can send a patch with a good
>> > argument for why it's worth changing in the commit message.
>> 
>> Sure! The cleanup is not part of the series but sticking to either one
>> of them in this patch is.
>
> I've lost track of what you're refering to.  If there is a problem with
> these patches, please comment on that in the right context.  But, this
> is an RFC, lots of things will change, and you would provide a more
> helpful code review by focusing on overall design issues etc. at this
> point.

I know this is a RFC. Changes to RFC include small changes too like
being consistent with using one type or the other. If you don't want that
in your reviews, that's fine too.


>> As for the argument, typedefs for structs are
>> discouraged as part of the coding style.
>> 
> Please...
>
> I encourage you to consider your audience.  I don't think you
> need to lecture me on the Linux kernel coding style.

I encourage you too to take a review as a review and not as a judgement of your
knowledge of the coding style or other abilities. You are discouraging people
from jumping in.

> -Christoffer

I started discussing with Jintack and reviewing this series so that I can understand
it better. If I have to constantly think that with every sentence I write I am judging
someone's abilities, or that I have to follow someone's rulebook for constructive comments,
well, thanks and good luck!