[RFC] Use para_fill instead of vmi_get_function for APIC ops

[RFC] Use para_fill instead of vmi_get_function for APIC ops

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 693 bytes --]

Hi Zach,

It seems to me that the APIC paravirt_ops should be filled by 
para_fill() instead of vmi_get_function().  vmi_get_function() returns a 
nop when the relocation type is NONE.  para_fill() leaves the native 
code in place.

The native version of the apic write ops is more or less *(APIC_BASE + 
reg) = value.  APIC_BASE is unknown to the ROM so it's impossible to 
simulate this in the ROM.

This means that a ROM has no choice but to do APIC emulation (or jump 
through seriously hairy loops to get the APIC mapped in it's address 
space).  Was this the intention?

N.B. attached patch is just to illustrate the point.  Has not even been 
compile tested.

Regards,

Anthony Liguori

[-- Attachment #2: vmi-apic-ops.diff --]
[-- Type: text/x-patch, Size: 552 bytes --]

--- linux-2.6.21-rc1/arch/i386/kernel/vmi.c~	2007-02-20 22:32:30.000000000 -0600
+++ linux-2.6.21-rc1/arch/i386/kernel/vmi.c	2007-02-26 17:58:18.000000000 -0600
@@ -852,9 +852,9 @@
 #endif
 
 #ifdef CONFIG_X86_LOCAL_APIC
-	paravirt_ops.apic_read = vmi_get_function(VMI_CALL_APICRead);
-	paravirt_ops.apic_write = vmi_get_function(VMI_CALL_APICWrite);
-	paravirt_ops.apic_write_atomic = vmi_get_function(VMI_CALL_APICWrite);
+	para_fill(apic_read, APICRead);
+	para_fill(apic_write, APICWrite);
+	para_fill(apic_write_atomic, APICWrite);
 #endif
 
 	/*

Re: [RFC] Use para_fill instead of vmi_get_function for APIC ops

[Zachary Amsden]

Anthony Liguori wrote:
> Hi Zach,
>
> It seems to me that the APIC paravirt_ops should be filled by 
> para_fill() instead of vmi_get_function().  vmi_get_function() returns 
> a nop when the relocation type is NONE.  para_fill() leaves the native 
> code in place.
>
> The native version of the apic write ops is more or less *(APIC_BASE + 
> reg) = value.  APIC_BASE is unknown to the ROM so it's impossible to 
> simulate this in the ROM.
>
> This means that a ROM has no choice but to do APIC emulation (or jump 
> through seriously hairy loops to get the APIC mapped in it's address 
> space).  Was this the intention?

No, but certainly the effect.  Actually, it is very easy to get the APIC 
mapped in the ROM address space without jumping through seriously hairy 
loops - we do it today in our hypervisor.

>
> N.B. attached patch is just to illustrate the point.  Has not even 
> been compile tested.


Patch looks good, thanks.   But the whole para_fill / vmi_get_function 
stuff could probably be done even cleaner.  It was just a helper at 
first to work around the awkward syntax, and it is still a bit ugly, but 
I haven't come up with a better solution yet, mostly because with the 
new inlining work Jeremy is doing, we might want to start doing 
selective inlining, in which case I'll have to go back over the code 
anyway to clean everything to get the logic right in all cases.

I assume this patch is signed-off-by you?  If so, I'll add it to my 
patch queue.

Zach

Re: [RFC] Use para_fill instead of vmi_get_function for APIC ops

[Anthony Liguori]

Zachary Amsden wrote:
> Anthony Liguori wrote:
>> Hi Zach,
>>
>> It seems to me that the APIC paravirt_ops should be filled by 
>> para_fill() instead of vmi_get_function().  vmi_get_function() 
>> returns a nop when the relocation type is NONE.  para_fill() leaves 
>> the native code in place.
>>
>> The native version of the apic write ops is more or less *(APIC_BASE 
>> + reg) = value.  APIC_BASE is unknown to the ROM so it's impossible 
>> to simulate this in the ROM.
>>
>> This means that a ROM has no choice but to do APIC emulation (or jump 
>> through seriously hairy loops to get the APIC mapped in it's address 
>> space).  Was this the intention?
>
> No, but certainly the effect.  Actually, it is very easy to get the 
> APIC mapped in the ROM address space without jumping through seriously 
> hairy loops - we do it today in our hypervisor.
>
>>
>> N.B. attached patch is just to illustrate the point.  Has not even 
>> been compile tested.
>
>
> Patch looks good, thanks.   But the whole para_fill / vmi_get_function 
> stuff could probably be done even cleaner.  It was just a helper at 
> first to work around the awkward syntax, and it is still a bit ugly, 
> but I haven't come up with a better solution yet, mostly because with 
> the new inlining work Jeremy is doing, we might want to start doing 
> selective inlining, in which case I'll have to go back over the code 
> anyway to clean everything to get the logic right in all cases.

It would be really great if one could write a ROM by just specifying a 
GetRelocationInfo function that always returns rel.type == NONE.  Right 
now, there are a half a dozen or so ops that have to be implemented b/c 
of the vmi_get_function stuff.

> I assume this patch is signed-off-by you?  If so, I'll add it to my 
> patch queue.

Yeah, but please make sure to test it.  I haven't at all.

Signed-off-by: Anthony Liguori <anthony@codemonkey.ws>

Thanks,

Anthony Liguori

> Zach
>

Re: [RFC] Use para_fill instead of vmi_get_function for APIC ops

[Zachary Amsden]

Anthony Liguori wrote:
>
> It would be really great if one could write a ROM by just specifying a 
> GetRelocationInfo function that always returns rel.type == NONE.  
> Right now, there are a half a dozen or so ops that have to be 
> implemented b/c of the vmi_get_function stuff.

Yes, I need to clean this up.  There are a couple other places where I 
took liberties and just did things that way because this was how our 
VMware ROM was implemented.  Just be sure, if you are going to implement 
a ROM, it has to be GPL'd now, otherwise the VMI code won't accept it.

>
>> I assume this patch is signed-off-by you?  If so, I'll add it to my 
>> patch queue.
>
> Yeah, but please make sure to test it.  I haven't at all.

It passes test level 1, which is good enough to make it into my patchset.

Zach

Re: [RFC] Use para_fill instead of vmi_get_function for APIC ops

[Jeremy Fitzhardinge]

Zachary Amsden wrote:
> Patch looks good, thanks.   But the whole para_fill / vmi_get_function
> stuff could probably be done even cleaner.  It was just a helper at
> first to work around the awkward syntax, and it is still a bit ugly,
> but I haven't come up with a better solution yet, mostly because with
> the new inlining work Jeremy is doing, we might want to start doing
> selective inlining, in which case I'll have to go back over the code
> anyway to clean everything to get the logic right in all cases.

Yes, my patching updates make pretty much all the pv_ops patchable,
including the apic ones.  The simple thing is to fill out paravirt_ops
with the appropriate pointers, then set the .patch operation to
paravirt_default_patch, which will basically turn them all into direct
calls.  If you want to start inlining stuff, then you can do that too.

    J

Re: [RFC] Use para_fill instead of vmi_get_function for APIC ops

[Anthony Liguori]

Zachary Amsden wrote:
> Anthony Liguori wrote:
>> Hi Zach,
>>
>> It seems to me that the APIC paravirt_ops should be filled by 
>> para_fill() instead of vmi_get_function().  vmi_get_function() 
>> returns a nop when the relocation type is NONE.  para_fill() leaves 
>> the native code in place.
>>
>> The native version of the apic write ops is more or less *(APIC_BASE 
>> + reg) = value.  APIC_BASE is unknown to the ROM so it's impossible 
>> to simulate this in the ROM.
>>
>> This means that a ROM has no choice but to do APIC emulation (or jump 
>> through seriously hairy loops to get the APIC mapped in it's address 
>> space).  Was this the intention?
>
> No, but certainly the effect.  Actually, it is very easy to get the 
> APIC mapped in the ROM address space without jumping through seriously 
> hairy loops - we do it today in our hypervisor.

I neglected to mention that I didn't want to use a memory hole.  One 
could allocate a small one to map the APIC but that seems to defeat the 
purpose of having a native ROM.

Regards,

Anthony Liguori

>>
>> N.B. attached patch is just to illustrate the point.  Has not even 
>> been compile tested.
>
>
> Patch looks good, thanks.   But the whole para_fill / vmi_get_function 
> stuff could probably be done even cleaner.  It was just a helper at 
> first to work around the awkward syntax, and it is still a bit ugly, 
> but I haven't come up with a better solution yet, mostly because with 
> the new inlining work Jeremy is doing, we might want to start doing 
> selective inlining, in which case I'll have to go back over the code 
> anyway to clean everything to get the logic right in all cases.
>
> I assume this patch is signed-off-by you?  If so, I'll add it to my 
> patch queue.
>
> Zach
>