Q: What is the struct kvm srcu protecting?

Q: What is the struct kvm srcu protecting?

[David Daney]

Hi,

I am working on the MIPS KVM port, and am trying to figure out under 
which circumstances do I need to srcu_read_lock()/srcu_read_unlock() the 
kvm->srcu.

I am looking at implementing something similar to arch/x86/kvm/x86.c at 
__msr_io(), where we see:

.
.
.
	idx = srcu_read_lock(&vcpu->kvm->srcu);
	for (i = 0; i < msrs->nmsrs; ++i)
		if (do_msr(vcpu, entries[i].index, &entries[i].data))
			break;
	srcu_read_unlock(&vcpu->kvm->srcu, idx);
.
.
.

Why is the srcu_read_lock() taken here?  I see no srcu_dereference() in 
the code path that would indicate the need for obtaining the lock.

I have a feeling that I am missing some essential concept about the 
design of this code, but I don't know what it is.

Can someone explain what is happening here?


Thanks in advance,
David Daney

Re: Q: What is the struct kvm srcu protecting?

[Marcelo Tosatti]

On Thu, May 02, 2013 at 11:22:52AM -0700, David Daney wrote:
> Hi,
> 
> I am working on the MIPS KVM port, and am trying to figure out under
> which circumstances do I need to srcu_read_lock()/srcu_read_unlock()
> the kvm->srcu.

For x86: kvm->srcu protects memory slot information (kvm->memslots) and
in-kernel MMIO/PIO address->device structure mapping (kvm->buses).
Search for synchronize_srcu_expedited() in virt/kvm/ to locate the
updaters.

> I am looking at implementing something similar to arch/x86/kvm/x86.c
> at __msr_io(), where we see:
> 
> .
> .
> .
> 	idx = srcu_read_lock(&vcpu->kvm->srcu);
> 	for (i = 0; i < msrs->nmsrs; ++i)
> 		if (do_msr(vcpu, entries[i].index, &entries[i].data))
> 			break;
> 	srcu_read_unlock(&vcpu->kvm->srcu, idx);
> .
> .
> .
> 
> Why is the srcu_read_lock() taken here?  I see no srcu_dereference()
> in the code path that would indicate the need for obtaining the
> lock.

        case KVM_GET_MSRS:
                r = msr_io(vcpu, argp, kvm_get_msr, 1);
                break;
        case KVM_SET_MSRS:
                r = msr_io(vcpu, argp, do_set_msr, 0);
                break;

to

int kvm_get_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
{
        return kvm_x86_ops->get_msr(vcpu, msr_index, pdata);
}

Somewhere down that path memslot information must be accessed.

> I have a feeling that I am missing some essential concept about the
> design of this code, but I don't know what it is.
> 
> Can someone explain what is happening here?

For x86 the usage is: 

VCPU CODE PATH
--------------

IOCTL(KVM_FD, KVM_VCPU_RUN)
ENTER-KERNEL
SRCU_READ_LOCK() 
	... large parte of vcpu context code performed with srcu lock held, 
	so that memory slot information can be used to access guest memory
	(gfn_to_memslot for example).
SRCU_READ_UNLOCK()
VMENTER
	while in guest mode srcu is not held so that updaters can make
	progress
VMEXIT
SRCU_READ_LOCK()
	back to vcpu context code

SRCU_READ_UNLOCK before return to userspace.

Also, when emulating HALT (kvm_vcpu_block) srcu is dropped.

UPDATERS
--------
See synchronize_srcu_expedited in virt/kvm/

Re: Q: What is the struct kvm srcu protecting?

[Gleb Natapov]

Hi David,

On Thu, May 02, 2013 at 10:48:36PM -0300, Marcelo Tosatti wrote:
> On Thu, May 02, 2013 at 11:22:52AM -0700, David Daney wrote:
> > Hi,
> > 
> > I am working on the MIPS KVM port, and am trying to figure out under
> > which circumstances do I need to srcu_read_lock()/srcu_read_unlock()
> > the kvm->srcu.
> 
Is your work somehow related to the work of Sanjay Lal that can be found
here: https://git.linux-mips.org/?p=ralf/upstream-sfr.git;a=summary?

Some clarification/addition to what Marcelo said below.

> For x86: kvm->srcu protects memory slot information (kvm->memslots) and
> in-kernel MMIO/PIO address->device structure mapping (kvm->buses).
> Search for synchronize_srcu_expedited() in virt/kvm/ to locate the
> updaters.
This is not only for x86. Any arch code that access memslots have to be
srcu read protected.

> 
> > I am looking at implementing something similar to arch/x86/kvm/x86.c
> > at __msr_io(), where we see:
> > 
> > .
> > .
> > .
> > 	idx = srcu_read_lock(&vcpu->kvm->srcu);
> > 	for (i = 0; i < msrs->nmsrs; ++i)
> > 		if (do_msr(vcpu, entries[i].index, &entries[i].data))
> > 			break;
> > 	srcu_read_unlock(&vcpu->kvm->srcu, idx);
> > .
> > .
> > .
> > 
> > Why is the srcu_read_lock() taken here?  I see no srcu_dereference()
> > in the code path that would indicate the need for obtaining the
> > lock.
> 
>         case KVM_GET_MSRS:
>                 r = msr_io(vcpu, argp, kvm_get_msr, 1);
>                 break;
>         case KVM_SET_MSRS:
>                 r = msr_io(vcpu, argp, do_set_msr, 0);
>                 break;
> 
> to
> 
> int kvm_get_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
> {
>         return kvm_x86_ops->get_msr(vcpu, msr_index, pdata);
> }
> 
> Somewhere down that path memslot information must be accessed.
> 
As Marcelo explained bellow on X86 srcu lock is held while vcpu loop is
running, so all the code that is usually called from vcpu does not do
its own locking. In rare cases that this code is called not from vcpu
main loop the locking is done explicitly on the high level.

This design is architecture choice. Other arches access memslot much
less and do locking as close to memslot access as possible. 

> > I have a feeling that I am missing some essential concept about the
> > design of this code, but I don't know what it is.
> > 
> > Can someone explain what is happening here?
> 
> For x86 the usage is: 
> 
> VCPU CODE PATH
> --------------
> 
> IOCTL(KVM_FD, KVM_VCPU_RUN)
> ENTER-KERNEL
> SRCU_READ_LOCK() 
> 	... large parte of vcpu context code performed with srcu lock held, 
> 	so that memory slot information can be used to access guest memory
> 	(gfn_to_memslot for example).
> SRCU_READ_UNLOCK()
> VMENTER
> 	while in guest mode srcu is not held so that updaters can make
> 	progress
> VMEXIT
> SRCU_READ_LOCK()
> 	back to vcpu context code
> 
> SRCU_READ_UNLOCK before return to userspace.
> 
> Also, when emulating HALT (kvm_vcpu_block) srcu is dropped.
> 
> UPDATERS
> --------
> See synchronize_srcu_expedited in virt/kvm/
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

Re: Q: What is the struct kvm srcu protecting?

[David Daney]

On 05/03/2013 03:51 AM, Gleb Natapov wrote:
> Hi David,
>
> On Thu, May 02, 2013 at 10:48:36PM -0300, Marcelo Tosatti wrote:
>> On Thu, May 02, 2013 at 11:22:52AM -0700, David Daney wrote:
>>> Hi,
>>>
>>> I am working on the MIPS KVM port, and am trying to figure out under
>>> which circumstances do I need to srcu_read_lock()/srcu_read_unlock()
>>> the kvm->srcu.
>>
> Is your work somehow related to the work of Sanjay Lal that can be found
> here: https://git.linux-mips.org/?p=ralf/upstream-sfr.git;a=summary?
>

It is related in that a single asm/uaip/kvm.h must be shared between the 
implementations.  It differs in that it is based on the MIPS-VZ hardware 
virtualization feature, where as Sanjay's code is a pure software solution.

If possible some code might be shared between the two, but it may end up 
looking somewhat like the x86 implementation where there are separate 
VMX and SVM implementations.


> Some clarification/addition to what Marcelo said below.

.
.
.

Thanks to all for setting me straight here.

David Daney

Re: Q: What is the struct kvm srcu protecting?

[Gleb Natapov]

On Fri, May 03, 2013 at 10:21:09AM -0700, David Daney wrote:
> On 05/03/2013 03:51 AM, Gleb Natapov wrote:
> >Hi David,
> >
> >On Thu, May 02, 2013 at 10:48:36PM -0300, Marcelo Tosatti wrote:
> >>On Thu, May 02, 2013 at 11:22:52AM -0700, David Daney wrote:
> >>>Hi,
> >>>
> >>>I am working on the MIPS KVM port, and am trying to figure out under
> >>>which circumstances do I need to srcu_read_lock()/srcu_read_unlock()
> >>>the kvm->srcu.
> >>
> >Is your work somehow related to the work of Sanjay Lal that can be found
> >here: https://git.linux-mips.org/?p=ralf/upstream-sfr.git;a=summary?
> >
> 
> It is related in that a single asm/uaip/kvm.h must be shared between
> the implementations.  It differs in that it is based on the MIPS-VZ
> hardware virtualization feature, where as Sanjay's code is a pure
> software solution.
> 
> If possible some code might be shared between the two, but it may
> end up looking somewhat like the x86 implementation where there are
> separate VMX and SVM implementations.
> 
Sanjay code already has such design and the reason he gave for it was upcoming
HW virtualization support.

--
			Gleb.